FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: replace charCodeAt with fixed Unicode by rickyes · Pull Request #32758 · nodejs/node · GitHub

/ node Public

lib: replace charCodeAt with fixed Unicode - #32758

Closed
rickyes wants to merge 2 commits into
nodejs:masterfrom
rickyes:charCodeAt
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

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 added the console Issues and PRs related to the console subsystem. label Apr 10, 2020

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 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 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 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 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 commented Apr 14, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30707/

BridgeAR reviewed May 3, 2020
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 3, 2020

Copy link
Copy Markdown
Member

I feel the same way as @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 commented May 25, 2020

Copy link
Copy Markdown
Member

agree with that

jasnell commented May 25, 2020

Copy link
Copy Markdown
Member

Not sure what the concerns are @BridgeAR ... this LGTM

nodejs-github-bot commented May 26, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31564/

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
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 commented Jun 19, 2020

Copy link
Copy Markdown
Member

Landed in 9918bdf

jasnell closed this Jun 19, 2020
rickyes deleted the charCodeAt branch June 19, 2020 14:49
codebytere pushed a commit that referenced this pull request Jun 22, 2020
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 mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
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
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
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 mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR left review comments
jasnell jasnell approved these changes
lpinca lpinca approved these changes
himself65 himself65 approved these changes
trivikr trivikr approved these changes

Assignees

No one assigned

Labels

console Issues and PRs related to the console subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL