src: fixup strings, general code cleanup #14937

jasnell · 2017-08-19T05:46:12Z

Some general source cleanups

Reduce duplicate strings
Add AsyncWrap::AddWrapMethods

Checklist

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

Affected core subsystem(s)

src

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base.

addaleax · 2017-08-19T12:34:29Z

src/async-wrap.h

@@ -87,13 +87,22 @@ class AsyncWrap : public BaseObject {
    PROVIDERS_LENGTH,
  };

+  enum Flags {


Since you’re passing it as ints, and since we have C++11 available: How about enum class JSMethodFlags { (which would implicitly have the underlying type int)?

Wrote it this way to be consistent with StreamBase::Flags. Converting these to enum class ... is a bit more involved than I'd like to get here. If we want to convert those, then we can do both AsyncWrap::Flags and StreamBase::Flags at the same time in a separate PR.

refack · 2017-08-19T20:26:51Z

src/async-wrap.cc

@@ -468,6 +468,13 @@ void AsyncWrap::QueueDestroyId(const FunctionCallbackInfo<Value>& args) {
  PushBackDestroyId(Environment::GetCurrent(args), args[0]->NumberValue());
 }

+void AsyncWrap::AddWrapMethods(Environment* env,


Can we bikeshed about the name.
IMHO Add -> Bind and Wrap -> AsyncHooks so BindAsyncHooksMethods

Add -> Bind

Why? I’d disagree.

Wrap -> AsyncHooks

Not the same things.

~~AddAsyncWrapMethods might be more explicit, if you like it.~~ Disregard that, this is static method anyway, so it would always seem redundant.

I felt AddWrapMethods doesn't convey the semantics clearly, so I tried to decostruct, and see what felt wrong.

Add -> Bind

Why? I’d disagree.

Add has (for me) the connotation of creating something new, and since env->SetProtoMethod AFAICT only creates JS binding for an existing native method Bind sounds more appropriate. But we can also extend the existing metaphor and use Set.

Wrap -> AsyncHooks

Not the same things.

Agreed, but AFAIK getAsyncId and asyncReset are new and only used in the context of the hooks so AsyncHooks felt more focused.
Maybe AsyncIDs is even better.

So to sum another option SetAsyncIDsMethods?

On third thought AsyncHooks is not good, although that's the "context", the methods are only used by the hooks, and are not the hooks. so:
+2 for AsyncIDs
+1 for AsyncWrap

I'm -1 on bikeshedding on this actually. Just don't see much point in doing so.

jasnell · 2017-08-20T16:33:14Z

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

jasnell · 2017-08-21T18:18:59Z

CI is green. One failure there is unrelated.

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base. PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

jasnell · 2017-08-23T17:38:18Z

Landed in 771a03d and 5e7c697

PR-URL: nodejs/node#14937 Reviewed-By: Anna Henningsen <[email protected]>

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base. PR-URL: nodejs/node#14937 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs/node#14937 Reviewed-By: Anna Henningsen <[email protected]>

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base. PR-URL: nodejs/node#14937 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base. PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base. PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base. PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2017-09-20T20:22:33Z

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

jasnell added 2 commits August 18, 2017 21:58

src: fixup strings, reduce duplication

9990f11

src: reduce code duplication

50524da

Adds `AsyncWrap::AddWrapMethods()` to add common methods to a `Local<FunctionTemplate>`. Follows same pattern as stream base.

nodejs-github-bot 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 Aug 19, 2017

jasnell requested a review from addaleax August 19, 2017 05:47

refack added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap labels Aug 19, 2017

addaleax approved these changes Aug 19, 2017

View reviewed changes

addaleax removed the async_hooks Issues and PRs related to the async hooks subsystem. label Aug 19, 2017

refack reviewed Aug 19, 2017

View reviewed changes

jasnell added a commit that referenced this pull request Aug 23, 2017

src: fixup strings, reduce duplication

771a03d

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

jasnell closed this Aug 23, 2017

addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017

src: fixup strings, reduce duplication

604e0c9

PR-URL: nodejs/node#14937 Reviewed-By: Anna Henningsen <[email protected]>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017

src: fixup strings, reduce duplication

cdc4a0f

PR-URL: nodejs/node#14937 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

src: fixup strings, reduce duplication

d5819db

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins pushed a commit that referenced this pull request Sep 11, 2017

src: fixup strings, reduce duplication

9b9932a

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

src: fixup strings, reduce duplication

5a05dfe

PR-URL: #14937 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fixup strings, general code cleanup #14937

src: fixup strings, general code cleanup #14937

jasnell commented Aug 19, 2017

addaleax Aug 19, 2017

jasnell Aug 20, 2017

refack Aug 19, 2017

addaleax Aug 19, 2017

addaleax Aug 19, 2017 •

edited

Loading

refack Aug 19, 2017

refack Aug 19, 2017

jasnell Aug 20, 2017

jasnell commented Aug 20, 2017

jasnell commented Aug 21, 2017

jasnell commented Aug 23, 2017

MylesBorins commented Sep 20, 2017

src: fixup strings, general code cleanup #14937

src: fixup strings, general code cleanup #14937

Conversation

jasnell commented Aug 19, 2017

Checklist

Affected core subsystem(s)

addaleax Aug 19, 2017

Choose a reason for hiding this comment

jasnell Aug 20, 2017

Choose a reason for hiding this comment

refack Aug 19, 2017

Choose a reason for hiding this comment

addaleax Aug 19, 2017

Choose a reason for hiding this comment

addaleax Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 19, 2017

Choose a reason for hiding this comment

refack Aug 19, 2017

Choose a reason for hiding this comment

jasnell Aug 20, 2017

Choose a reason for hiding this comment

jasnell commented Aug 20, 2017

jasnell commented Aug 21, 2017

jasnell commented Aug 23, 2017

MylesBorins commented Sep 20, 2017

addaleax Aug 19, 2017 •

edited

Loading