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.

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

timers: modified comment to try/finally #15205

Closed

Conversation

Moniarchy
Copy link

Replaced try/catch with try/finally in comments, line 744.

Fixes: #14308

Checklist
Affected core subsystem(s)

Replaced try/catch with try/finally in comments, line 744.

Fixes: nodejs#14308
@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 5, 2017
@jasnell
Copy link
Member

jasnell commented Sep 5, 2017

Not even sure if the comment applies any more since try blocks should be optimized now. But, in general, LGTM

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2017

There is actually already a open PR for this: #14314

@refack
Copy link
Contributor

refack commented Sep 5, 2017

Hello @Moniarchy and welcome. Thank for the contribution 🥇
As @BridgeAR pointed out there is an open PR to fix the comment. But there's an opportunity for you to follow up on the comments made before an remove reference to try/finally optimization. Currently the reason to use runCallback is the explicit code that triggers the callback with specific number of arguments:

node/lib/timers.js

Lines 775 to 794 in b8c142c

function runCallback(timer) {
const argv = timer._argv;
const argc = argv ? argv.length : 0;
if (typeof timer._callback !== 'function')
return promiseResolve(timer._callback, argv[0]);
switch (argc) {
// fast-path callbacks with 0-3 arguments
case 0:
return timer._callback();
case 1:
return timer._callback(argv[0]);
case 2:
return timer._callback(argv[0], argv[1]);
case 3:
return timer._callback(argv[0], argv[1], argv[2]);
// more than 3 arguments run slower with .apply
default:
return Function.prototype.apply.call(timer._callback, timer, argv);
}
}

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

This got superseded by a2b6872

@Moniarchy thanks for your contribution anyway! Looking forward to your next PR.

@BridgeAR BridgeAR closed this Sep 9, 2017
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.

Outdated comment in timers
5 participants