Skip to content

Outdated comment in timers #14308

Closed
Closed
Outdated comment in timers#14308
Labels
good first issueIssues that are suitable for first-time contributors.timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().wipIssues and PRs that are still a work in progress.

Description

@benjamingr
benjamingr
opened on Jul 16, 2017

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.

Activity

  1. benjamingr
    added
    good first issueIssues that are suitable for first-time contributors.
    timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().
    on Jul 16, 2017

  2. timcosta commented on Jul 16, 2017

    @timcosta
    timcosta
    on Jul 16, 2017
    Contributor

    I'm on this.

  3. targos commented on Jul 16, 2017

    @targos
    targos
    on Jul 16, 2017
    Member

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

  4. timcosta commented on Jul 17, 2017

    @timcosta
    timcosta
    on Jul 17, 2017
    Contributor

    PR updated

  5. benjamingr commented on Jul 17, 2017

    @benjamingr
    benjamingr
    on Jul 17, 2017
    MemberAuthor

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

  6. targos commented on Jul 17, 2017

    @targos
    targos
    on Jul 17, 2017
    Member

    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.

  7. Fishrock123 commented on Jul 17, 2017

    @Fishrock123
    Fishrock123
    on Jul 17, 2017
    Contributor

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

    (And also correct IIRC)

    EDIT: ok maybe not with TurboFan

  8. sam0410 commented on Aug 19, 2017

    @sam0410
    sam0410
    on Aug 19, 2017

    Hi !
    Can I make the PR for this?
    Thanks

  9. Moniarchy
    added a commit that references this issue on Sep 5, 2017

    timers: modified comment to try/finally

    b8c142c
  10. refack
    added
    wipIssues and PRs that are still a work in progress.
    on Sep 5, 2017

  11. BridgeAR commented on Sep 9, 2017

    @BridgeAR
    BridgeAR
    on Sep 9, 2017
    Member

    This got fixed by a2b6872

  12. BridgeAR
    closed this as completedon Sep 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    good first issueIssues that are suitable for first-time contributors.timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().wipIssues and PRs that are still a work in progress.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions