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

Outdated comment in timers #14308

Closed
benjamingr opened this issue Jul 16, 2017 · 8 comments
Closed

Outdated comment in timers #14308

benjamingr opened this issue Jul 16, 2017 · 8 comments
Labels
good first issue Issues that are suitable for first-time contributors. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. wip Issues and PRs that are still a work in progress.

Comments

@benjamingr
Copy link
Member

Currently, the timers contain an outdated comment in https://github.com/nodejs/node/blob/master/lib/timers.js#L742 referring to a try/catch statement that is not there.

We should remove it.
@benjamingr benjamingr added good first issue Issues that are suitable for first-time contributors. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jul 16, 2017
@timcosta
Copy link
Contributor

I'm on this.

@timcosta timcosta mentioned this issue Jul 16, 2017
Closed
@targos
Copy link
Member

targos commented Jul 16, 2017

It's still there. The comment should just say try/finally instead of try/catch

@timcosta
Copy link
Contributor

PR updated

@benjamingr
Copy link
Member Author

@targos yes, but the "allow it to be optimized" part is no longer true with TurboFan

@targos
Copy link
Member

targos commented Jul 17, 2017

True, but that's not the reason you gave in the OP. I still think the comment is relevant because it explains why the call is made in another function. If that is not relevant anymore, we shouldn't just remove the comment but revert the optimization altogether.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 17, 2017 •
edited
Loading

This comment is intentional in it's current position: 6f75b66#diff-0a5d4868b2b9b17cf9e2c11f1bd1311eR545

(And also correct IIRC)

EDIT: ok maybe not with TurboFan

@sam0410
Copy link

sam0410 commented Aug 19, 2017 •
edited
Loading

Hi !
Can I make the PR for this?
Thanks

Moniarchy added a commit to Moniarchy/node that referenced this issue Sep 5, 2017
@Moniarchy
timers: modified comment to try/finally
b8c142c 
Replaced try/catch with try/finally in comments, line 744.

Fixes: nodejs#14308
@Moniarchy Moniarchy mentioned this issue Sep 5, 2017
Closed
1 task
@refack refack added the wip Issues and PRs that are still a work in progress. label Sep 5, 2017
@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

This got fixed by a2b6872

@BridgeAR BridgeAR closed this as completed Sep 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
7 participants
@refack @Fishrock123 @benjamingr @targos @timcosta @BridgeAR @sam0410