Skip to content

tools: fix js2c regression - #27980

Closed
refack wants to merge 2 commits into
nodejs:masterfrom
refack:fix-js2c-regression
Closed

tools: fix js2c regression#27980
refack wants to merge 2 commits into
nodejs:masterfrom
refack:fix-js2c-regression

Conversation

@refack

@refackrefack commented May 30, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Fix a regression that was introduced in #25518 and causes the binary to bloat by ~10%.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@refack
refack requested review from cclauss and joyeecheungMay 30, 2019 15:56
@refackrefack added python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. regression Issues related to regressions. labels May 30, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
refack
refack commented May 30, 2019
View reviewed changes
Comment threadtools/js2c.py Outdated
@refackrefack mentioned this pull request May 30, 2019
Merged
3 tasks
@refack
refackforce-pushed the fix-js2c-regression branch from f473fa2 to e302589CompareMay 30, 2019 21:13
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@refack
refackforce-pushed the fix-js2c-regression branch from e302589 to 84aca5cCompareMay 30, 2019 21:30
@nodejs-github-bot

This comment has been minimized.

Sign in to view
joyeecheung
joyeecheung reviewed May 30, 2019
View reviewed changes
Comment threadtest/cctest/test_per_process.cc Outdated

@joyeecheungjoyeecheungMay 30, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The builds are failing because this is private. I guess it would also make sense to add a public method to just return the source in a copy, though.

@refackrefackMay 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it a friend
https://github.com/nodejs/node/blob/84aca5ca594f86ce9b6b86e6af039e78f90deaef/src/node_native_module.h#L89
and it compiles & passes with MSVC, now I need to figure out gcc & clang 🤷‍♂

@refackrefackMay 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it was a namespace issue
https://github.com/nodejs/node/blob/c178598d309319e9a9e9692d6560a9205cd1b307/src/node_native_module.h#L89

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented May 31, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23505/

targos
targos approved these changes May 31, 2019
View reviewed changes
Comment threadnode.gyp Outdated
Comment threadnode.gyp Outdated
bnoordhuis
bnoordhuis approved these changes Jun 1, 2019
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work.

Comment threadtools/js2c.py Outdated

@bnoordhuisbnoordhuisJun 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you wrap this, e.g. by putting the for i in ... on its own line, lining it up with the e in [encoded_source?

@refackrefackJun 1, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@refack
refackforce-pushed the fix-js2c-regression branch from 8494bba to 6b0295cCompareJune 1, 2019 14:36
@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23548/

@Trott

Trott commented Jun 2, 2019

Copy link
Copy Markdown
Member

Landed in cb92d24

@TrottTrott closed this Jun 2, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 2, 2019
@refack@Trott
tools: fix js2c regression
cb92d24
PR-URL: nodejs#27980
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack
refack deleted the fix-js2c-regression branch June 2, 2019 16:18
@refack
refack removed the request for review from cclaussJune 2, 2019 16:18
targos pushed a commit that referenced this pull request Jun 3, 2019
@refack@targos
tools: fix js2c regression
2ce24a9
PR-URL: #27980
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targostargos mentioned this pull request Jun 3, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@targostargostargos approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

pythonPRs and issues that require attention from people who are familiar with Python.regressionIssues related to regressions.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@refack@nodejs-github-bot@Trott@bnoordhuis@targos@joyeecheung