Skip to content

lib: replace charCodeAt with fixed Unicode - #32758

Closed
rickyes wants to merge 2 commits into
nodejs:masterfrom
rickyes:charCodeAt
Closed

lib: replace charCodeAt with fixed Unicode#32758
rickyes wants to merge 2 commits into
nodejs:masterfrom
rickyes:charCodeAt

Conversation

@rickyes

@rickyes rickyes commented Apr 10, 2020

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Apr 10, 2020
@addaleax

addaleax commented Apr 10, 2020

Copy link
Copy Markdown
Member

Can you explain what the motivation here is? If the goal is avoiding a few calls to .charCodeAt(), I don’t think it’s going to make a measurable impact.
@rickyes

rickyes commented Apr 10, 2020

Copy link
Copy Markdown
Contributor Author

Can you explain what the motivation here is? If the goal is avoiding a few calls to .charCodeAt(), I don’t think it’s going to make a measurable impact.

@addaleax Yes, in order to avoid unnecessary calls to charCodeAt.
@rickyes

rickyes commented Apr 10, 2020

Copy link
Copy Markdown
Contributor Author

@addaleax Or we can move to internal/constants.js ？

  // Alphabet chars.
  CHAR_UPPERCASE_A: 65, /* A */
  CHAR_LOWERCASE_A: 97, /* a */
  CHAR_UPPERCASE_Z: 90, /* Z */
  CHAR_LOWERCASE_Z: 122, /* z */
  CHAR_UPPERCASE_C: 67, /* C */
  CHAR_LOWERCASE_B: 98, /* b */
  CHAR_LOWERCASE_E: 101, /* e */
  CHAR_LOWERCASE_N: 110, /* n */
@addaleax

addaleax commented Apr 10, 2020

Copy link
Copy Markdown
Member

@rickyes Yeah, I guess that would be consistent… I’m good with that 👍

I’m also good with doing nothing and keeping the code as straightforward as possible.
@rickyes

rickyes commented Apr 10, 2020

Copy link
Copy Markdown
Contributor Author

@rickyes Yeah, I guess that would be consistent… I’m good with that 👍

I’m also good with doing nothing and keeping the code as straightforward as possible.

done.
@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30707/
BridgeAR
BridgeAR reviewed May 3, 2020
View reviewed changes

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

I would rather keep the code as it is right now but I won't block this, since other collaborators seem to agree with this change.
@addaleax

addaleax commented May 3, 2020

Copy link
Copy Markdown
Member

I feel the same way as @BridgeAR.
@BridgeAR

BridgeAR commented May 25, 2020

Copy link
Copy Markdown
Member

@jasnell @lpinca @trivikr @himself65 PTAL: it would be good to get some feedback about the mentioned concerns.
@himself65

himself65 commented May 25, 2020

Copy link
Copy Markdown
Member

agree with that
@jasnell

jasnell commented May 25, 2020

Copy link
Copy Markdown
Member

Not sure what the concerns are @BridgeAR ... this LGTM
@nodejs-github-bot

nodejs-github-bot commented May 26, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31564/
@BridgeAR
BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
jasnell pushed a commit that referenced this pull request Jun 19, 2020
@rickyes @jasnell
lib: replace charCodeAt with fixed Unicode
9918bdf 
PR-URL: #32758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@jasnell

jasnell commented Jun 19, 2020

Copy link
Copy Markdown
Member

Landed in 9918bdf
@jasnell jasnell closed this Jun 19, 2020
@rickyes
rickyes deleted the charCodeAt branch June 19, 2020 14:49
codebytere pushed a commit that referenced this pull request Jun 22, 2020
@rickyes @codebytere
lib: replace charCodeAt with fixed Unicode
ec01867 
PR-URL: #32758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@rickyes @codebytere
lib: replace charCodeAt with fixed Unicode
2430c42 
PR-URL: #32758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
@rickyes @codebytere
lib: replace charCodeAt with fixed Unicode
c8c2885 
PR-URL: #32758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
codebytere pushed a commit that referenced this pull request Jul 12, 2020
@rickyes @codebytere
lib: replace charCodeAt with fixed Unicode
0197ea4 
PR-URL: #32758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@rickyes @codebytere
lib: replace charCodeAt with fixed Unicode
fdd3f3d 
PR-URL: #32758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

console Issues and PRs related to the console subsystem.

8 participants

@rickyes @addaleax @nodejs-github-bot @BridgeAR @himself65 @jasnell @lpinca @trivikr