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: don't store one-use strings in variable #21876

Closed
wants to merge 1 commit into from
Closed

src: don't store one-use strings in variable #21876

wants to merge 1 commit into from

Conversation

maclover7
Copy link
Contributor

Move strings that are used only once to their call-sites, don't store
in a variable. What most other places in src/cares_wrap.cc do, as well as other files in src.

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 19, 2018
@addaleax
Copy link
Member

@maclover7 I’d also label this with src as the subsystem, btw. The main purpose of adding those to commit messages is to make changes to certain subsystems easier to spot in changelogs, but this does not actually change the behaviour of the dns module :)

@maclover7
src: don't store one-use strings in variable
801ee85 
Move strings that are used only once to their call-sites, don't store
in a variable.
@maclover7 maclover7 changed the title dns: don't store one-use strings in variable src: don't store one-use strings in variable Jul 19, 2018
@maclover7
Copy link
Contributor Author

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

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2018
@maclover7
Copy link
Contributor Author

Landed in de8a8d0

@maclover7 maclover7 closed this Jul 22, 2018
@maclover7 maclover7 deleted the jm-mv-str branch July 22, 2018 13:49
maclover7 added a commit that referenced this pull request Jul 22, 2018
@maclover7
src: don't store one-use strings in variable
de8a8d0 
Move strings that are used only once to their call-sites, don't store
in a variable.

PR-URL: #21876
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@maclover7 maclover7 restored the jm-mv-str branch July 22, 2018 15:42
@maclover7 maclover7 reopened this Jul 22, 2018
@maclover7 maclover7 closed this Jul 22, 2018
@maclover7 maclover7 deleted the jm-mv-str branch July 22, 2018 15:42
targos pushed a commit that referenced this pull request Jul 24, 2018
@maclover7 @targos
src: don't store one-use strings in variable
be75795 
Move strings that are used only once to their call-sites, don't store
in a variable.

PR-URL: #21876
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
This was referenced Aug 2, 2018
Closed
Closed
This was referenced Jun 18, 2021
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@JungMinu JungMinu JungMinu approved these changes

@devsnek devsnek devsnek approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. 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.
9 participants
@maclover7 @nodejs-github-bot @addaleax @jasnell @TimothyGu @cjihrig @JungMinu @devsnek @trivikr