★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24079Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

remove unused catch bindings #24079

Merged
merged 12 commits into from Nov 6, 2018

Conversation

Reviewers

@sam-github sam-github

@geek geek

Assignees
No one assigned
Labels
dont-land-on-v6.x dont-land-on-v8.x land-on-v10.x lib / src refactor to ES6+
Projects
None yet
Milestone
No milestone
8 participants
@cjihrig @nodejs-github-bot @refack @bmeurer @Jimbly @codebytere @sam-github @geek
@cjihrig
Copy link
Contributor

cjihrig commented Nov 4, 2018 •
edited

NOTE: I didn't update the unused bindings in the stream subsystem, as I assume that would break readable-stream.

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

@nodejs-github-bot nodejs-github-bot added the lib / src label Nov 4, 2018

@cjihrig cjihrig added dont-land-on-v8.x dont-land-on-v6.x labels Nov 4, 2018

@sam-github

sam-github approved these changes Nov 4, 2018
View changes

Copy link
Member

sam-github left a comment

It would also be handy to have eslint check for unused catch bindings, so they don't creep back into the code.

@refack refack added the refactor to ES6+ label Nov 4, 2018

@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

cjihrig commented Nov 4, 2018

@sam-github enabled linting for unused catch bindings. Good idea.
@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Nov 4, 2018 •
edited

I'm +1 on this (both for readability, and explicit-y), but there was the "backporting" argument made in another PR.

@bmeurer does not binding the unused exception object provide any performance benefit (or will in the future 🤞 )?

It would also be handy to have eslint check for unused catch bindings, so they don't creep back into the code.

I would actually suggest the opposite, lint for these and force the author explicitly comment why it's Ok (out of scope of this PR).
@bmeurer

This comment has been minimized.

Sign in to view
Copy link
Member

bmeurer commented Nov 4, 2018

@refack V8 doesn't need to allocate the catch context in that case, which makes the exception object available. TurboFan will probably optimize that away anyways, but for bytecode it's a nice saving.
@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

cjihrig commented Nov 6, 2018 •
edited

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

EDIT: Green CI: https://ci.nodejs.org/job/node-test-pull-request/18368/
@geek

geek approved these changes Nov 6, 2018
View changes

cjihrig added some commits Nov 4, 2018

@cjihrig
fs: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
9d68e56
@cjihrig
assert: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
39a0272
@cjihrig
events: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
57c5c7d
@cjihrig
bootstrap: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
81b335d
@cjihrig
esm: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
a64ca71
@cjihrig
inspector: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
fcb8bf1
@cjihrig
v8_prof_polyfill: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
30f58b9
@cjihrig
querystring: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
ab14ad1
@cjihrig
repl: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
a7ba5fa
@cjihrig
test: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
a74b4a0
@cjihrig
benchmark: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
5407690
@cjihrig
tools: lint for unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
Loading status checks…
5e6193f

@cjihrig cjihrig force-pushed the cjihrig:catch branch from 540b3cd to 5e6193f Nov 6, 2018

@cjihrig cjihrig merged commit 5e6193f into nodejs:master Nov 6, 2018

@cjihrig cjihrig deleted the cjihrig:catch branch Nov 6, 2018

@Jimbly

This comment has been minimized.

Sign in to view
Copy link
Contributor

Jimbly commented Nov 6, 2018 •
edited

This PR appears to be crashing Travis CI on PRs since this was merged? https://travis-ci.com/nodejs/node/builds/90427486

$ make lint
make[1]: Entering directory `/home/travis/build/nodejs/node'
Running JS linter...
/home/travis/build/nodejs/node/.eslintrc.js:20
    } catch {
            ^
SyntaxError: Unexpected token {
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:599:28)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at module.exports (/home/travis/build/nodejs/node/tools/node_modules/eslint/node_modules/require-uncached/index.js:28:9)

@yehiyam yehiyam referenced this pull request Nov 6, 2018

Closed

test: add test for deepEqual Float32Array byte length difference #24164

3 of 3 tasks complete

targos added a commit that referenced this pull request Nov 6, 2018

@cjihrig @targos
fs: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
d59dc0d

targos added a commit that referenced this pull request Nov 6, 2018

@cjihrig @targos
assert: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
685724b
@codebytere

This comment has been minimized.

Sign in to view
Copy link
Member

codebytere commented Nov 29, 2018 •
edited

@cjihrig i see don't-lands for v6.x and v8.x; should this also not be backported to v10.x?
@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

cjihrig commented Nov 29, 2018

I believe optional catch bindings are supported in Node 10, so it can be backported.

@codebytere codebytere added the land-on-v10.x label Nov 29, 2018

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
fs: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
e486960

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
assert: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
b472b63

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
events: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
b885695

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
bootstrap: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
693b2e7

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
esm: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
ca4fd89

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
inspector: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
5c12f4a

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
v8_prof_polyfill: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
24e3be7

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
querystring: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
0b42564

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
repl: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
bbd38ea

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
test: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
7f2ce6e

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
benchmark: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
6bfc3e4

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
tools: lint for unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
Loading status checks…
4782b3c

codebytere added a commit that referenced this pull request Nov 29, 2018

@cjihrig @codebytere
tools: lint for unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
Loading status checks…
517d6f3

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
fs: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
a5462ba

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
assert: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
dedf0e5

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
events: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
e2b96da

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
bootstrap: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
0600e75

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
esm: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
1491f32

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
inspector: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
d79d978

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
v8_prof_polyfill: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
80ab31e

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
querystring: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
8001bee

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
repl: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
4b456d5

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
test: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
78be477

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
benchmark: remove unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
eed47b0

MylesBorins added a commit that referenced this pull request Dec 3, 2018

@cjihrig @MylesBorins
tools: lint for unused catch bindings 
PR-URL: #24079
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
a67e04e

@codebytere codebytere referenced this pull request Dec 3, 2018

Merged

v10.14.2 proposal #24727

@ZYSzys ZYSzys referenced this pull request Dec 18, 2018

Merged

chore: remove unused catch bindings #16121

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.