src: minor c++ refactors to module_wrap #15515

addaleax · 2017-09-21T00:05:29Z

Move module_map to Environment instead of having it be global state
std::map → std::unordered_map
Remove one level of indirection for the map values
Clean up empty vectors in module_map
Call Reset() on all persistent handles in resolve_cache_
Add a missing HandleScope to ModuleWrap::~ModuleWrap()

Checklist

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

Affected core subsystem(s)

src/module_wrap

bnoordhuis · 2017-09-21T10:30:58Z

src/module_wrap.cc

@@ -27,15 +27,13 @@ using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Module;
 using v8::Object;
-using v8::Persistent;
 using v8::Promise;
 using v8::ScriptCompiler;
 using v8::ScriptOrigin;
 using v8::String;
 using v8::Value;

 static const char* EXTENSIONS[] = {".mjs", ".js", ".json", ".node"};


Since you're here, can you make this static const char *const? The pointed-to strings are const now but the array itself is not.

bnoordhuis · 2017-09-21T10:33:26Z

src/module_wrap.cc

-  std::vector<ModuleWrap*>* same_hash = module_map_[module->GetIdentityHash()];
-  auto it = std::find(same_hash->begin(), same_hash->end(), this);
+  Isolate* isolate = Isolate::GetCurrent();
+  Environment* env = Environment::GetCurrent(isolate);


ModuleWrap is a BaseObject, right? You can just use env() here. Same goes for the constructor: env()->isolate(). There are probably quite a few more places that could be updated.

bnoordhuis · 2017-09-21T10:44:49Z

src/module_wrap.cc

+  if (it != same_hash.end()) {
+    same_hash.erase(it);
+    if (same_hash.empty())
+      env->module_map.erase(module->GetIdentityHash());
  }


Correct me if I'm wrong but if you used a std::unordered_multimap, you could drop the extra indirection of the std::vector. You could then simply look up env->module_map.equal_range(module->GetIdentityHash()).

Yes, good idea – done.

bnoordhuis · 2017-09-21T10:47:56Z

src/module_wrap.cc

@@ -244,7 +243,7 @@ MaybeLocal<Module> ModuleWrap::ResolveCallback(Local<Context> context,
  }

  Local<Promise> resolve_promise =
-      dependent->resolve_cache_[specifier_std]->Get(iso);
+      dependent->resolve_cache_[specifier_std].Get(iso);


This habit of using iso when the rest of the code base consistently uses isolate... urgh.

whats the problem? / there are not style docs saying to use isolate?

That doesn't mean we shouldn't try to keep things consistent.

@Fishrock123 I agree consistency is nice, but every pull request I do seems to have style errors and there is no guide; I will continue to bring this up as long as I see comments like "urgh." for innocent mistakes.

Have to agree with @bmeck here. There really ought to be a style guide that makes these things more obvious.

Not everything needs to be spelled out. When you start making changes to a code base you're not familiar with, you first peek around to get a feel for the house style. 10 minutes of browsing would show that we use isolate everywhere and iso nowhere.

@bnoordhuis you have even in the past called PRs of mine stupid. That line of reasoning and interactions like the one above make me want things spelled out to avoid such behavior.

If it's any consolation to you, I'm also disappointed none of the reviewers called it out.

It is not. Mistakes are made by everyone and we should work together, not bring pessimism into things if it can be avoided.

Fwiw I was pretty sure I pushed commits to the original PR that did that particular style change, because I knew it wasn’t obvious and I think @bmeck said he preferred that to an endless list of style nits? These probably just got lost during some force push or sth.

jasnell · 2017-09-21T13:08:05Z

LGTM once @bnoordhuis' comments are addressed

bmeck

Seems fine to me, though my C++ isn't as strong as others so I am mostly deferring to them for structure choices.

addaleax · 2017-09-21T19:18:50Z

@bnoordhuis Updated, PTAL

addaleax · 2017-09-22T15:22:16Z

~~CI: https://ci.nodejs.org/job/node-test-commit/12522/~~ missed linter error
new CI: https://ci.nodejs.org/job/node-test-commit/12554/

- Move `module_map` to `Environment` instead of having it be global state - `std::map` → `std::unordered_map` - Remove one level of indirection for the map values - Clean up empty vectors in `module_map` - Call `Reset()` on all persistent handles in `resolve_cache_` - Add a missing `HandleScope` to `ModuleWrap::~ModuleWrap()` PR-URL: nodejs#15515 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

addaleax · 2017-09-23T15:01:52Z

Landed in 150b8f7

- Move `module_map` to `Environment` instead of having it be global state - `std::map` → `std::unordered_map` - Remove one level of indirection for the map values - Clean up empty vectors in `module_map` - Call `Reset()` on all persistent handles in `resolve_cache_` - Add a missing `HandleScope` to `ModuleWrap::~ModuleWrap()` PR-URL: #15515 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

- Move `module_map` to `Environment` instead of having it be global state - `std::map` → `std::unordered_map` - Remove one level of indirection for the map values - Clean up empty vectors in `module_map` - Call `Reset()` on all persistent handles in `resolve_cache_` - Add a missing `HandleScope` to `ModuleWrap::~ModuleWrap()` PR-URL: nodejs/node#15515 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

- Move `module_map` to `Environment` instead of having it be global state - `std::map` → `std::unordered_map` - Remove one level of indirection for the map values - Clean up empty vectors in `module_map` - Call `Reset()` on all persistent handles in `resolve_cache_` - Add a missing `HandleScope` to `ModuleWrap::~ModuleWrap()` PR-URL: #15515 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

MylesBorins · 2017-10-17T18:33:43Z

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

addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 21, 2017

addaleax requested a review from bmeck September 21, 2017 00:05

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 21, 2017

bnoordhuis reviewed Sep 21, 2017

View reviewed changes

jasnell approved these changes Sep 21, 2017

View reviewed changes

bmeck approved these changes Sep 21, 2017

View reviewed changes

addaleax force-pushed the module-refactor branch from f7e0884 to 569daf0 Compare September 21, 2017 19:18

TimothyGu approved these changes Sep 21, 2017

View reviewed changes

addaleax force-pushed the module-refactor branch from 1c171c1 to e9ff842 Compare September 23, 2017 14:03

addaleax force-pushed the module-refactor branch from e9ff842 to e66a05d Compare September 23, 2017 14:03

addaleax closed this Sep 23, 2017

addaleax deleted the module-refactor branch September 23, 2017 15:01

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Uh oh!

src: minor c++ refactors to module_wrap #15515

src: minor c++ refactors to module_wrap #15515

Uh oh!

Conversation

addaleax commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 21, 2017

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 21, 2017

Uh oh!

addaleax commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Sep 23, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Uh oh!

addaleax commented Sep 21, 2017 •

edited

Loading

bnoordhuis Sep 21, 2017 •

edited

Loading

addaleax commented Sep 22, 2017 •

edited

Loading