Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools,test: var to const #13732

Merged
merged 7 commits into from
Jul 2, 2017
Merged

tools,test: var to const #13732

merged 7 commits into from
Jul 2, 2017

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jun 17, 2017 •
edited
Loading

As far as I know using const is preferable in almost all situations, so this converts probably all var declarations to const, if possible.

If this is not what you want, I'm happy to close this again or split into smaller parts.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools,test

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem. child_process Issues and PRs related to the child_process subsystem. cluster Issues and PRs related to the cluster subsystem. crypto Issues and PRs related to the crypto subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. domain Issues and PRs related to the domain subsystem. v8 engine Issues and PRs related to the V8 dependency. labels Jun 17, 2017
@mscdex
Copy link
Contributor

mscdex commented Jun 17, 2017

Even though we're TurboFan by default now, I would still be cautious about permanent deopts when using const/let. You might want to at least run a make test-check-deopts | grep -ivP ' <(?:\/|SharedFunctionInfo)' and see what aborted/disabled optimizations shows up. Some are expected though, such as the deopts in domain and vm.

@BridgeAR
Copy link
Member Author

@mscdex I did that and there was no new constant deopt.

@mscdex
Copy link
Contributor

mscdex commented Jun 17, 2017 •
edited
Loading

Actually I just checked current master and it looks like make test-check-deopts is broken again :-(

Typically you will see a lot of output with that command I gave, because tests tend to have a lot of deopts (although the grep ignores the content of those deopt messages it still prints the filenames).

EDIT: It looks like I was mistaken. I'm guessing I just haven't ran that since the switch to V8 5.9 and TurboFan by default. There's only about 6 perm deopts as of this writing and I believe they're all inside the tests themselves.

@vsemozhetbyt
Copy link
Contributor

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

@vsemozhetbyt vsemozhetbyt added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem. console Issues and PRs related to the console subsystem. dns Issues and PRs related to the dns subsystem. events Issues and PRs related to the events subsystem / EventEmitter. fs Issues and PRs related to the fs subsystem / file system. https Issues or PRs related to the https subsystem. module Issues and PRs related to the module subsystem. process Issues and PRs related to the process subsystem. net Issues and PRs related to the net subsystem. path Issues and PRs related to the path subsystem. url Issues and PRs related to the legacy built-in url module. util Issues and PRs related to the built-in util module. querystring Issues and PRs related to the built-in querystring module. labels Jun 17, 2017
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
@BridgeAR @addaleax
tools: change var to const in ./license2rtf
149ae78 
PR-URL: nodejs#13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
@BridgeAR @addaleax
test: change var to const in ./common
abd7cc3 
PR-URL: nodejs#13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/html
079b570 
PR-URL: nodejs#13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
@BridgeAR @addaleax
tools: change var to const in ./eslint-rules
3fe619b 
PR-URL: nodejs#13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax addaleax mentioned this pull request Jul 3, 2017
Merged
@joaocgreis
Copy link
Member

@refack If I'm not missing anything, this only influences make doc and and the licence displayed by the MSI (since common and eslint run in CI). https://nodejs.org/download/nightly/v9.0.0-nightly20170703fe730d34ce/ already has this change, the docs folder and the MSI seem to have been correctly generated.

@refack
Copy link
Contributor

refack commented Jul 4, 2017

@refack If I'm not missing anything, this only influences make doc and and the licence displayed by the MSI (since common and eslint run in CI). https://nodejs.org/download/nightly/v9.0.0-nightly20170703fe730d34ce/ already has this change, the docs folder and the MSI seem to have been correctly generated.

Thanks!

addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/addon-verify
5453886 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/json
6822f88 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/preprocess
fa6482a 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
tools: change var to const in ./license2rtf
fa3686c 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
test: change var to const in ./common
ffe9bb1 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/html
6f1cfa7 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@BridgeAR @addaleax
tools: change var to const in ./eslint-rules
67d6251 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/addon-verify
b89c27d 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/json
148f49f 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/preprocess
f3bff93 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
tools: change var to const in ./license2rtf
7ed7b22 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
test: change var to const in ./common
8edde98 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
tools: change var to const in ./doc/html
d454add 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@BridgeAR @addaleax
tools: change var to const in ./eslint-rules
d69527f 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
tools: change var to const in ./doc/addon-verify
7de10a2 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
tools: change var to const in ./doc/json
6dc3f98 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
tools: change var to const in ./doc/preprocess
5ece9c7 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
tools: change var to const in ./license2rtf
e959e2f 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
test: change var to const in ./common
ea63d7f 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
tools: change var to const in ./doc/html
9559c89 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@BridgeAR @Fishrock123
tools: change var to const in ./eslint-rules
1aa6bcd 
PR-URL: #13732
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BridgeAR BridgeAR deleted the var-to-const branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@refack refack refack approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees

@tniessen tniessen

Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@BridgeAR @mscdex @vsemozhetbyt @gibfahn @refack @jasnell @cjihrig @joaocgreis @tniessen @nodejs-github-bot