★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21876Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

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

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@jasnell jasnell

@addaleax addaleax

@TimothyGu TimothyGu

@cjihrig cjihrig

@JungMinu JungMinu

@devsnek devsnek

@trivikr trivikr

Assignees
No one assigned
Labels
C++ author ready lib / src
Projects
None yet
Milestone
No milestone
9 participants
@maclover7 @nodejs-github-bot @addaleax @jasnell @TimothyGu @cjihrig @JungMinu @devsnek @trivikr
@maclover7
Copy link
Member

maclover7 commented Jul 19, 2018

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
@devsnek

devsnek approved these changes Jul 19, 2018
View changes

@addaleax

addaleax approved these changes Jul 19, 2018
View changes

@addaleax addaleax added C++ lib / src labels Jul 19, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Jul 19, 2018

@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 :)
@trivikr

trivikr approved these changes Jul 19, 2018
View changes

@JungMinu

JungMinu approved these changes Jul 19, 2018
View changes

@jasnell

jasnell approved these changes Jul 19, 2018
View changes

@maclover7
src: don't store one-use strings in variable 
Move strings that are used only once to their call-sites, don't store
in a variable.
Loading status checks…
801ee85

@maclover7 maclover7 force-pushed the maclover7:jm-mv-str branch from 6d09116 to 801ee85 Jul 19, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

maclover7 commented Jul 19, 2018

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

TimothyGu approved these changes Jul 19, 2018
View changes

@cjihrig

cjihrig approved these changes Jul 19, 2018
View changes

@maclover7 maclover7 added the author ready label Jul 20, 2018

@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member Author

maclover7 commented Jul 22, 2018

Landed in de8a8d0

@maclover7 maclover7 closed this Jul 22, 2018

@maclover7 maclover7 deleted the maclover7:jm-mv-str branch Jul 22, 2018

maclover7 added a commit that referenced this pull request Jul 22, 2018

@maclover7
src: don't store one-use strings in variable 
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>
Loading status checks…
de8a8d0

@maclover7 maclover7 restored the maclover7:jm-mv-str branch Jul 22, 2018

@maclover7 maclover7 reopened this Jul 22, 2018

@maclover7 maclover7 closed this Jul 22, 2018

@maclover7 maclover7 deleted the maclover7:jm-mv-str branch Jul 22, 2018

targos added a commit that referenced this pull request Jul 24, 2018

@maclover7 @targos
src: don't store one-use strings in variable 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
be75795

@targos targos referenced this pull request Jul 31, 2018

Merged

v10.8.0 proposal #22040

This was referenced Aug 1, 2018

Closed
Closed
Closed
Closed

This was referenced Aug 2, 2018

Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.