Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@yorkie
Copy link
Contributor

@yorkie yorkie commented Sep 11, 2016

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

@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
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
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

Labels

timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.