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: use existing strings over creating new ones #14587

Closed
wants to merge 2 commits into from
Closed

src: use existing strings over creating new ones #14587

wants to merge 2 commits into from
+4 −10

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 2, 2017

This is a very very minor change.

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

src/tcp_wrap.cc

@addaleax
src: use existing strings over creating new ones
73dd304 
This is a very very minor change.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem. labels Aug 2, 2017
@mscdex
Copy link
Contributor

mscdex commented Aug 2, 2017

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/9446/

@tniessen
Copy link
Member

tniessen commented Aug 2, 2017

CI seems to hiccup, new attempt: https://ci.nodejs.org/job/node-test-pull-request/9449/

tniessen
tniessen reviewed Aug 2, 2017
View reviewed changes
src/tcp_wrap.cc
@@ -77,13 +77,9 @@ void TCPWrap::Initialize(Local<Object> target,
// Init properties
t->InstanceTemplate()->Set(String::NewFromUtf8(env->isolate(), "reading"),
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to add "reading" to env.h as well? Are we trying to keep that list small?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s there for strings that would otherwise be re-created multiple times in the lifetime of a Node instance, but this appears to be the only usage of it in src/.

@addaleax
Copy link
Member Author

addaleax commented Aug 7, 2017

Landed in efea3fc, 3f98b0f

@addaleax addaleax closed this Aug 7, 2017
@addaleax addaleax deleted the tcp-existing-strings branch August 7, 2017 12:57
addaleax added a commit that referenced this pull request Aug 7, 2017
@addaleax
src: use existing strings over creating new ones
efea3fc 
This is a very very minor change.

PR-URL: #14587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 7, 2017
@tniessen @addaleax
src: reuse 'ondone' string in node_crypto.cc
3f98b0f 
PR-URL: #14587
Reviewed-By: Anna Henningsen <[email protected]>
addaleax added a commit that referenced this pull request Aug 7, 2017
@addaleax
src: use existing strings over creating new ones
6ae6469 
This is a very very minor change.

PR-URL: #14587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 7, 2017
@tniessen @addaleax
src: reuse 'ondone' string in node_crypto.cc
264e434 
PR-URL: #14587
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@addaleax @MylesBorins
src: use existing strings over creating new ones
08e7545 
This is a very very minor change.

PR-URL: #14587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@addaleax @MylesBorins
src: use existing strings over creating new ones
071018e 
This is a very very minor change.

PR-URL: #14587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
@addaleax @MylesBorins
src: use existing strings over creating new ones
440b545 
This is a very very minor change.

PR-URL: #14587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@addaleax @MylesBorins
src: use existing strings over creating new ones
fe6735c 
This is a very very minor change.

PR-URL: #14587
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request Sep 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @mscdex @tniessen @MylesBorins @jasnell @TimothyGu @cjihrig @nodejs-github-bot