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

src: remove unused Converter object by addaleax · Pull Request #26243 · nodejs/node · GitHub

/ node Public

src: remove unused Converter object - #26243

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:unused-converter
ConversationCommits (1)ChecksFiles changed
Closed

src: remove unused Converter object#26243
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:unused-converter

Conversation

addaleax commented Feb 21, 2019

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

nodejs-github-bot commented Feb 21, 2019

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2691/pipeline

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Feb 21, 2019
bnoordhuis approved these changes Feb 21, 2019
View reviewed changes
Comment thread src/node_i18n.cc

CHECK_GE(args.Length(), 3); // Converter, Buffer, Flags

Converter utf8("utf8");

bnoordhuis Feb 21, 2019

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 guess this has the side effect of verifying that ICU is in a sane state? It'll CHECK when ucnv_open() fails.

But if that's the reason why it's there, it really could use an explaining comment. cc @jasnell.

addaleax Feb 28, 2019

Copy link
Copy Markdown
Member Author

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 assumed (and still do) that this is a leftover from when this code was written, tbh.

jasnell Mar 1, 2019

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 think @addaleax is right

addaleax commented Feb 28, 2019

Copy link
Copy Markdown
Member Author

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 28, 2019

addaleax commented Mar 1, 2019

Copy link
Copy Markdown
Member Author

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

addaleax commented Mar 1, 2019

Copy link
Copy Markdown
Member Author

Landed in 7f8010b

addaleax closed this Mar 1, 2019
addaleax deleted the unused-converter branch March 1, 2019 20:41
addaleax added a commit that referenced this pull request Mar 1, 2019
src: remove unused Converter object
7f8010b 
PR-URL: #26243
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
src: remove unused Converter object
c37b679 
PR-URL: #26243
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR mentioned this pull request Mar 4, 2019
Merged
kissofire mentioned this pull request Mar 7, 2021
Open
aliceUnhinged613 mentioned this pull request Mar 7, 2021
Open
ron-a20 mentioned this pull request Mar 8, 2021
Open
sitedata mentioned this pull request Mar 8, 2021
Open
meghasfdc mentioned this pull request Mar 8, 2021
Open
137717unity mentioned this pull request Mar 8, 2021
Open
Rednickle mentioned this pull request Mar 8, 2021
Open
This was referenced Mar 11, 2021
Open
Open
This was referenced May 25, 2021
Open
Open
Open
Open
Open
Open
This was referenced May 26, 2021
Open
Open
Open
Open
This was referenced Apr 7, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Closed
snyk-bot mentioned this pull request Jan 30, 2023
Open
wedataintelligence mentioned this pull request Jan 30, 2023
Open
snyk-bot mentioned this pull request Jan 30, 2023
Open
molllyn1 mentioned this pull request Jan 31, 2023
Open
snyk-bot mentioned this pull request Jan 31, 2023
Open
baby636 mentioned this pull request Jan 31, 2023
Open
This was referenced Jan 31, 2023
Open
Open
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

jasnell jasnell left review comments
bnoordhuis bnoordhuis approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL