src: fixup strings, general code cleanup#14937

Closed

jasnell wants to merge 2 commits intonodejs:masterfrom

jasnell:fixup-strings

Member

jasnell commented Aug 19, 2017

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

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++ lib / src labels

jasnell requested a review from addaleax

August 19, 2017 05:47

refack added async_hooks async_wrap labels

addaleax approved these changes

View reviewed changes

src/async-wrap.h

                   PROVIDERS_LENGTH,
                 };
+                enum Flags {

Member

addaleax Aug 19, 2017

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)?

Member Author

jasnell Aug 20, 2017

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.

addaleax removed the async_hooks label

refack reviewed

View reviewed changes

src/async-wrap.cc

                 PushBackDestroyId(Environment::GetCurrent(args), args[0]->NumberValue());
               }
+              void AsyncWrap::AddWrapMethods(Environment* env,

Contributor

refack Aug 19, 2017

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

Member

addaleax Aug 19, 2017

Add -> Bind

Why? I’d disagree.

Wrap -> AsyncHooks

Not the same things.

Member

addaleax Aug 19, 2017 •

edited

Loading

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

Contributor

refack Aug 19, 2017

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?

Contributor

refack Aug 19, 2017

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

Member Author

jasnell Aug 20, 2017

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

Member Author

jasnell commented Aug 20, 2017

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

Member Author

jasnell commented Aug 21, 2017

CI is green. One failure there is unrelated.

jasnell added a commit that referenced this pull request


          src: fixup strings, reduce duplication

771a03d

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added a commit that referenced this pull request


          src: reduce code duplication

5e7c697

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

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Member Author

jasnell commented Aug 23, 2017

Landed in 771a03d and 5e7c697

jasnell closed this

addaleax pushed a commit to addaleax/ayo that referenced this pull request


          src: fixup strings, reduce duplication

604e0c9

PR-URL: nodejs/node#14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax pushed a commit to addaleax/ayo that referenced this pull request


          src: reduce code duplication

a1936cf

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 <anna@addaleax.net>

addaleax pushed a commit to ayojs/ayo that referenced this pull request


          src: fixup strings, reduce duplication

cdc4a0f

PR-URL: nodejs/node#14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax pushed a commit to ayojs/ayo that referenced this pull request


          src: reduce code duplication

af8449e

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 <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          src: fixup strings, reduce duplication

d5819db

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          src: reduce code duplication

a02823c

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

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins mentioned this pull request

v8.5.0 proposal #15308

Merged

MylesBorins pushed a commit that referenced this pull request


          src: fixup strings, reduce duplication

9b9932a

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          src: reduce code duplication

12245ae

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

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          src: fixup strings, reduce duplication

5a05dfe

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          src: reduce code duplication

8cee5d6

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

PR-URL: #14937
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added the dont-land-on-v6.x label

Contributor

MylesBorins commented Sep 20, 2017

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

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

Labels