lib: remove bootstrap global context indirection #5881
Conversation
Fishrock123
added
c++
|
LGTM, I suppose. |
|
LGTM |
|
agree, the "use strict" and then old convention of using |
|
LGTM |
|
@jasnell This poses no risk to lts, but at the same time no benefit either. CI before landing: https://ci.nodejs.org/job/node-test-pull-request/2055/ |
|
Part of me wonders if this should be passed at all though? I guess this makes I could probably just do it in C++ if that is even less confusing. |
|
@Fishrock123 it was always using one passed via C++, it is just a convention/style gotcha |
|
@bmeck I mean the only place we actually need it passed is to set |
|
@Fishrock123 ah, I see yes, you could manually set |
Fishrock123
force-pushed
the
less-context-indirection
branch
from
d074e34
to
1dc1278
Compare
|
Moved to c++, CI: https://ci.nodejs.org/job/node-test-pull-request/2056/ |
|
I marked it don't land on v4.x because the underlying refactoring of src/node.js hasn't been pulled back. I'm not sure if we should. @nodejs/lts |
|
@jasnell This should take no effort to backport? |
|
It's more a question over whether we should, not about how much effort is involved. @nodejs/lts ... what do you think? |
Fishrock123
force-pushed
the
less-context-indirection
branch
from
1dc1278
to
e69686c
Compare
| @@ -208,7 +207,6 @@ | |||
|
|
|||
| function setupGlobalVariables() { | |||
| global.process = process; | |||
| global.global = global; | |||
There was a problem hiding this comment.
From #5888 (comment)
Looks this is was duplicated and unnecessary.
|
Could I get this re-LTGM'd, since I changed most of it from the original? |
|
LGTM |
PR-URL: nodejs#5881 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123
force-pushed
the
less-context-indirection
branch
from
e69686c
to
21d66d6
Compare
| Local<Value> arg = env->process_object(); | ||
| f->Call(global, 1, &arg); | ||
| f->Call(Null(env->isolate()), ARRAY_SIZE(&arg), &arg); |
There was a problem hiding this comment.
I think this code was changed after I reviewed it? Because ARRAY_SIZE(&arg) doesn't do what you think it does.
|
With Ben's comment, perhaps we should hold off on this on v5? |
|
@bnoordhuis I'm not sure what you mean by that? Trevor suggested it. It was originally |
|
The problem is that Local<Value> argv[] = { a, b, c }; // ARRAY_SIZE(&argv) is *not* 3 like you would expect |
|
@Fishrock123 this is not landing cleanly on |
|
Really? I'll take a look when get home. @thealphanerd this is not important for the release. |
PR-URL: nodejs#5881 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Conflicts: lib/internal/bootstrap_node.js src/node.cc
PR-URL: #5881 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Conflicts: lib/internal/bootstrap_node.js src/node.cc
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
Affected core subsystem(s)
lib,bootstrap
Description of change
From an irc discussion with @bmeck, this is less confusing than relying on what the "this" context is.