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.

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: fix comments re PER_ISOLATE macros #12899

Merged
merged 1 commit into from May 10, 2017

Conversation

@joshgav
Copy link
Member

commented May 8, 2017

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

Update and clarify the comments on the PER_ISOLATE macros in env and move to right before the macros they describe.

src/env.h Outdated
//
// In each macro, `V` is expected to be the name of a C function which accepts
// the number of arguments provided in each tuple in the macro body, typically
// two. The named function will be invoked against each tuple.

This comment has been minimized.

Copy link
@addaleax

addaleax May 8, 2017

Member

V is (almost?) always another macro, not a C function. :) (You can change s/invoked/expanded for that in the last sentence, if you don’t have a better word in mind.)

This comment has been minimized.

Copy link
@joshgav

joshgav May 8, 2017

Author Member

I guess it could technically be anything which could syntactically be followed by a parameter list? I'll rephrase.

src/env.h Outdated
@@ -64,6 +56,18 @@ namespace node {
#define NODE_PUSH_VAL_TO_ARRAY_MAX 8
#endif

// PER_ISOLATE_* macros: we're going slightly crazy with macros here but the

This comment has been minimized.

Copy link
@addaleax

addaleax May 8, 2017

Member

I know it’s copy-pasted, but “crazy” isn’t really an appropriate word here… not sure how to phrase it better, but if nothing else comes to your mind, you can go with something like “dark magic”. ;)

This comment has been minimized.

Copy link
@AndreasMadsen

AndreasMadsen May 8, 2017

Member

wild is my personal substitution.

This comment has been minimized.

Copy link
@joshgav

joshgav May 8, 2017

Author Member

I took out that sentence entirely and it doesn't seem we lose anything, see what you think.

@joshgav joshgav force-pushed the joshgav:env-comment-cleanup branch May 8, 2017

@addaleax
Copy link
Member

left a comment

thank you a lot! :)

@danbev
danbev approved these changes May 9, 2017
@cjihrig
cjihrig approved these changes May 9, 2017
@jasnell
jasnell approved these changes May 9, 2017
src: fix comments re PER_ISOLATE macros
PR-URL: #12899
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>

@joshgav joshgav force-pushed the joshgav:env-comment-cleanup branch to dd6e3f6 May 10, 2017

@joshgav

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

Landed in dd6e3f6. Thanks!

@joshgav joshgav closed this May 10, 2017

@joshgav joshgav merged commit dd6e3f6 into nodejs:master May 10, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
src: fix comments re PER_ISOLATE macros
PR-URL: nodejs#12899
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Marking dont-land as it conflicts. Feel free to backport if you'd like to.

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