Skip to content

lib: adding optemisation notes, leaving var in place in js_stream_socket.js - #30415

Closed
ryanj wants to merge 1 commit into
nodejs:masterfrom
ryanj:replace-var-with-let-nceu19
Closed

lib: adding optemisation notes, leaving var in place in js_stream_socket.js#30415
ryanj wants to merge 1 commit into
nodejs:masterfrom
ryanj:replace-var-with-let-nceu19

Conversation

@ryanj

@ryanjryanj commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

CC @mcollina. Adding comments to explain why var is preferred over let in this loop

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@ryanjryanj changed the title Adding optemisation notes, leaving var in place for nowlib: adding optemisation notes, leaving var in place in js_stream_socket.jsNov 12, 2019
@ryanj
ryanjforce-pushed the replace-var-with-let-nceu19 branch from 8b2faa6 to ec6ad97CompareNovember 12, 2019 15:51
@jasnelljasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 12, 2019
Comment threadlib/internal/js_stream_socket.js Outdated

@trivikrtrivikr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with suggestion posted by mscdex

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ryanj
ryanjforce-pushed the replace-var-with-let-nceu19 branch from ec6ad97 to 82689aaCompareNovember 24, 2019 03:02
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadlib/internal/js_stream_socket.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit:

Suggested change
// Prefer `var` over `let` for performance optimization
// Prefer `var` over `let` for performance optimization.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @lundibundi. fixed

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reworded, as build results warned: Line should be <= 72 columns

@ryanj
ryanjforce-pushed the replace-var-with-let-nceu19 branch from 82689aa to 296adc4CompareNovember 26, 2019 03:38
Leaving var in place of let for performance optimization in short loops in hot paths. Added comments explaining why.
@ryanj
ryanjforce-pushed the replace-var-with-let-nceu19 branch from 296adc4 to 0c2391bCompareNovember 26, 2019 05:21
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

gireeshpunathil pushed a commit that referenced this pull request Nov 26, 2019
Leaving var in place of let for performance optimization
in short loops in hot paths. Added comments explaining why.
PR-URL: #30415
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@gireeshpunathil

Copy link
Copy Markdown
Member

landed in 79e86ac.

thanks for the contribution!

addaleax pushed a commit that referenced this pull request Nov 30, 2019
Leaving var in place of let for performance optimization
in short loops in hot paths. Added comments explaining why.
PR-URL: #30415
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Dec 1, 2019
Leaving var in place of let for performance optimization
in short loops in hot paths. Added comments explaining why.
PR-URL: #30415
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeARBridgeAR mentioned this pull request Dec 3, 2019
@BethGriggsBethGriggs mentioned this pull request Dec 9, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
Leaving var in place of let for performance optimization
in short loops in hot paths. Added comments explaining why.
PR-URL: #30415
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Dec 23, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@ryanj@nodejs-github-bot@gireeshpunathil@mcollina@mscdex@jasnell@BridgeAR@lundibundi@trivikr