lib: remove bootstrap global context indirection by Fishrock123 · Pull Request #5881 · nodejs/node

Fishrock123 · 2016-03-23T22:11:56Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?

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.

bnoordhuis · 2016-03-24T09:37:29Z

LGTM, I suppose.

cjihrig · 2016-03-24T12:58:18Z

LGTM

bmeck · 2016-03-24T13:53:28Z

agree, the "use strict" and then old convention of using this as global was a big double take for me.

jasnell · 2016-03-24T14:43:37Z

LGTM

Fishrock123 · 2016-03-24T14:53:24Z

@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/

Fishrock123 · 2016-03-24T14:55:48Z

Part of me wonders if this should be passed at all though? I guess this makes global in this file use the passed reference and not actually the global one?

I could probably just do it in C++ if that is even less confusing.

bmeck · 2016-03-24T14:59:25Z

@Fishrock123 it was always using one passed via C++, it is just a convention/style gotcha

Fishrock123 · 2016-03-24T15:02:36Z

@bmeck I mean the only place we actually need it passed is to set <global>.global = <global> so we can access it in any scope via global, right?

bmeck · 2016-03-24T15:05:53Z

@Fishrock123 ah, I see yes, you could manually set global via C++ on the Context's Global() and remove this.

Fishrock123 · 2016-03-24T15:38:35Z

Moved to c++, CI: https://ci.nodejs.org/job/node-test-pull-request/2056/

jasnell · 2016-03-24T18:53:20Z

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

Fishrock123 · 2016-03-24T19:00:15Z

@jasnell This should take no effort to backport?

jasnell · 2016-03-24T19:04:43Z

It's more a question over whether we should, not about how much effort is involved. @nodejs/lts ... what do you think?

Fishrock123 · 2016-03-24T19:05:46Z

lib/internal/bootstrap_node.js

From #5888 (comment)

Looks this is was duplicated and unnecessary.

Fishrock123 · 2016-03-30T19:01:59Z

Could I get this re-LTGM'd, since I changed most of it from the original?

cjihrig · 2016-03-30T19:05:52Z

LGTM

PR-URL: nodejs#5881 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

bnoordhuis · 2016-03-31T10:38:38Z

src/node.cc

+
  Local<Value> arg = env->process_object();
-  f->Call(global, 1, &arg);
+  f->Call(Null(env->isolate()), ARRAY_SIZE(&arg), &arg);


I think this code was changed after I reviewed it? Because ARRAY_SIZE(&arg) doesn't do what you think it does.

evanlucas · 2016-03-31T17:34:51Z

With Ben's comment, perhaps we should hold off on this on v5?

Fishrock123 · 2016-04-04T16:28:29Z

@bnoordhuis I'm not sure what you mean by that? Trevor suggested it. It was originally 2. Does this break something?

bnoordhuis · 2016-04-04T20:37:38Z

The problem is that ARRAY_SIZE(&arg) evaluates to sizeof(&arg) / sizeof(*&arg). It works by accident because sizeof(Local<Value>*) == sizeof(Local<Value>) but it breaks with arrays:

Local<Value> argv[] = { a, b, c };  // ARRAY_SIZE(&argv) is *not* 3 like you would expect

MylesBorins · 2016-04-05T17:55:07Z

@Fishrock123 this is not landing cleanly on v5.x

Fishrock123 · 2016-04-05T18:03:09Z

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Conflicts: lib/internal/bootstrap_node.js src/node.cc

PR-URL: #5881 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Conflicts: lib/internal/bootstrap_node.js src/node.cc

Fishrock123 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 Mar 23, 2016

jasnell added the dont-land-on-v4.x label Mar 24, 2016

Fishrock123 force-pushed the less-context-indirection branch from d074e34 to 1dc1278 Compare March 24, 2016 15:37

Fishrock123 force-pushed the less-context-indirection branch from 1dc1278 to e69686c Compare March 24, 2016 19:04

Fishrock123 reviewed Mar 24, 2016
View reviewed changes

lib/internal/bootstrap_node.js Outdated

Copy link

Contributor Author

Fishrock123 Mar 24, 2016

Choose a reason for hiding this comment

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

From #5888 (comment)

Looks this is was duplicated and unnecessary.

joshgav mentioned this pull request Mar 30, 2016

node: Update strings node.js -> bootstrap_node.js #5962

Closed

3 tasks

lib: remove bootstrap global context indirection

21d66d6

PR-URL: nodejs#5881 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Fishrock123 force-pushed the less-context-indirection branch from e69686c to 21d66d6 Compare March 30, 2016 23:59

Fishrock123 merged commit 21d66d6 into nodejs:master Mar 31, 2016

bnoordhuis reviewed Mar 31, 2016
View reviewed changes

Fishrock123 mentioned this pull request Apr 5, 2016

lib: remove bootstrap global context indirection #6062

Closed

2 tasks

This was referenced Apr 20, 2016

V5.10.2 proposal #6311

Closed

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

Uh oh!

Conversation

Fishrock123 commented Mar 23, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Mar 24, 2016

Uh oh!

cjihrig commented Mar 24, 2016

Uh oh!

bmeck commented Mar 24, 2016

Uh oh!

jasnell commented Mar 24, 2016

Uh oh!

Fishrock123 commented Mar 24, 2016

Uh oh!

Fishrock123 commented Mar 24, 2016

Uh oh!

bmeck commented Mar 24, 2016

Uh oh!

Fishrock123 commented Mar 24, 2016

Uh oh!

bmeck commented Mar 24, 2016

Uh oh!

Fishrock123 commented Mar 24, 2016

Uh oh!

jasnell commented Mar 24, 2016

Uh oh!

Fishrock123 commented Mar 24, 2016

Uh oh!

jasnell commented Mar 24, 2016

Uh oh!

Fishrock123 Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 30, 2016

Uh oh!

cjihrig commented Mar 30, 2016

Uh oh!

bnoordhuis Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Mar 31, 2016

Uh oh!

Fishrock123 commented Apr 4, 2016

Uh oh!

bnoordhuis commented Apr 4, 2016

Uh oh!

MylesBorins commented Apr 5, 2016

Uh oh!

Fishrock123 commented Apr 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants