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

timers: length == 0 is untouchable because type checking #8487

Closed
wants to merge 1 commit into from
Closed

timers: length == 0 is untouchable because type checking #8487

wants to merge 1 commit into from

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented Sep 11, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

Description of change

In setTimetout, setInterval and setImmediate functions, the case 0 on arguments.length are untouchable because an TypeError would be threw and not able to run at that line.

@yorkie yorkie added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 11, 2016
@Trott
Copy link
Member

Trott commented Sep 11, 2016

LGTM if CI is green

@cjihrig
Copy link
Contributor

cjihrig commented Sep 11, 2016

LGTM

@yorkie
Copy link
Contributor Author

yorkie commented Sep 11, 2016

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

@lpinca
Copy link
Member

lpinca commented Sep 11, 2016

Commit subject line is a bit too long, can I suggest "timers: remove unreachable code"?
Change LGTM though.

@yorkie yorkie force-pushed the timers/fix-untouchable branch from c5ad7fc to 42fbed1 Compare September 11, 2016 06:51
@yorkie
Copy link
Contributor Author

yorkie commented Sep 11, 2016

Thank you @lpinca changed to your suggestion :)

@addaleax
Copy link
Member

LGTM, CI failures look unrelated (one test-crypto-timing-safe-equal failure)

@Fishrock123
Copy link
Contributor

LGTM

@Fishrock123
Copy link
Contributor

Actually, isn't 1 also impossible to get to then?

@yorkie
Copy link
Contributor Author

yorkie commented Sep 11, 2016

@Fishrock123 setTimeout(fn) should be reachable to case 1, the TypeError should be only thrown when the callback is not a function.

@Fishrock123
Copy link
Contributor

Ah yes. Misunderstood.

@yorkie
Copy link
Contributor Author

yorkie commented Sep 15, 2016

Landed at 9b0246b :)

@yorkie yorkie closed this Sep 15, 2016
yorkie added a commit that referenced this pull request Sep 15, 2016
@yorkie
timers: remove unreachable code
9b0246b 
PR-URL: #8487
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@yorkie yorkie deleted the timers/fix-untouchable branch September 15, 2016 06:28
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@yorkie @Fishrock123
timers: remove unreachable code
02da155 
PR-URL: #8487
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yorkie @Trott @cjihrig @lpinca @addaleax @Fishrock123 @MylesBorins