Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: remove unused strings from env.h #17643

Closed
wants to merge 1 commit into from
Closed

src: remove unused strings from env.h #17643

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes (presumably)
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 13, 2017
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/14789/

@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. fast-track PRs that do not need to wait for 48 hours to land. labels Dec 13, 2017
@gireeshpunathil
Copy link
Member

@addaleax - just wondering how these strings came into being in the first place? they were used once and orphaned by the PRs over time?

@addaleax
Copy link
Member Author

@gireeshpunathil Yeah, I would assume exactly that. 👍

TimothyGu
TimothyGu approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

That's a lot of strings.

maclover7
maclover7 approved these changes Dec 13, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes

Maybe a little bit of a stretch, but is there any kind of lint rule we can apply to check for this?

@lpinca
Copy link
Member

lpinca commented Dec 13, 2017

The best one was nice_string.

@addaleax
Copy link
Member Author

Landed in de22009

@addaleax addaleax closed this Dec 16, 2017
@addaleax addaleax deleted the env-string-cleanup branch December 16, 2017 15:30
addaleax added a commit that referenced this pull request Dec 16, 2017
@addaleax
src: remove unused strings from env.h
de22009 
PR-URL: #17643
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@addaleax @MylesBorins
src: remove unused strings from env.h
fb76ec4 
PR-URL: #17643
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@addaleax @MylesBorins
src: remove unused strings from env.h
6f520e3 
PR-URL: #17643
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

Setting Don't land for both 6.x and 8.x. If you would like to backport or do the same work on one of the LTS lines feel free, but I don't think it is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@maclover7 maclover7 maclover7 approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.