From 2d6d4e6003c47ec5cf789e680d267d1a043b7cc6 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 12 Feb 2018 23:12:02 +0200 Subject: [PATCH] c++ feedback --- src/env.h | 7 ++++++ src/module_wrap.cc | 58 ++++++++++++++++++++++++---------------------- src/module_wrap.h | 9 ++++--- 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/env.h b/src/env.h index 95548c0900ea13..f13add295838a2 100644 --- a/src/env.h +++ b/src/env.h @@ -608,6 +608,13 @@ class Environment { std::unordered_multimap module_map; + struct PackageConfig { + bool exists; + bool has_main; + std::string main; + }; + std::unordered_map package_json_cache; + inline double* heap_statistics_buffer() const; inline void set_heap_statistics_buffer(double* pointer); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 2c52bdc7fa3470..9dd1d7d64f9bc9 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -480,38 +480,37 @@ Maybe CheckFile(const std::string& path, uv_fs_req_cleanup(&fs_req); if (is_directory) { - uv_fs_close(nullptr, &fs_req, fd, nullptr); + CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, fd, nullptr)); uv_fs_req_cleanup(&fs_req); return Nothing(); } if (opt == CLOSE_AFTER_CHECK) { - uv_fs_close(nullptr, &fs_req, fd, nullptr); + CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, fd, nullptr)); uv_fs_req_cleanup(&fs_req); } return Just(fd); } -PackageConfig emptyPackage = { false, false, "" }; -std::unordered_map pjson_cache_; +const Environment::PackageConfig& GetPackageConfig(Environment* env, + const std::string path) { + static const Environment::PackageConfig kEmptyPackage = { false, false, "" }; -PackageConfig& GetPackageConfig(Environment* env, const std::string path) { - auto existing = pjson_cache_.find(path); - if (existing != pjson_cache_.end()) { + auto existing = env->package_json_cache.find(path); + if (existing != env->package_json_cache.end()) return existing->second; - } Maybe check = CheckFile(path, LEAVE_OPEN_AFTER_CHECK); - if (check.IsNothing()) { - return pjson_cache_[path] = - emptyPackage; - } + if (check.IsNothing()) + return kEmptyPackage; Isolate* isolate = env->isolate(); - Local context = isolate->GetCurrentContext(); + + v8::HandleScope handle_scope(isolate); + std::string pkg_src = ReadFile(check.FromJust()); uv_fs_t fs_req; - uv_fs_close(nullptr, &fs_req, check.FromJust(), nullptr); + CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, check.FromJust(), nullptr)); uv_fs_req_cleanup(&fs_req); // It's not okay for the called of this method to not be able to tell @@ -523,27 +522,28 @@ PackageConfig& GetPackageConfig(Environment* env, const std::string path) { pkg_src.c_str(), v8::NewStringType::kNormal, pkg_src.length()).ToLocal(&src)) { - return pjson_cache_[path] = - emptyPackage; + return env->package_json_cache[path] = kEmptyPackage; } - Local pkg_json; - if (!JSON::Parse(context, src).ToLocal(&pkg_json) || !pkg_json->IsObject()) - return pjson_cache_[path] = - emptyPackage; + Local pkg_json_v; + Local pkg_json; + + if (!JSON::Parse(env->context(), src).ToLocal(&pkg_json_v) || + !pkg_json_v->ToObject(env->context()).ToLocal(&pkg_json)) + return kEmptyPackage; // Exception pending. + Local pkg_main; bool has_main = false; std::string main_std; - if (pkg_json.As()->Get(context, env->main_string()) + if (pkg_json->Get(env->context(), env->main_string()) .ToLocal(&pkg_main) && pkg_main->IsString()) { has_main = true; Utf8Value main_utf8(isolate, pkg_main.As()); main_std = std::string(*main_utf8, main_utf8.length()); } - PackageConfig pjson = { true, has_main, main_std }; - return pjson_cache_[path] = - pjson; + Environment::PackageConfig pjson = { true, has_main, main_std }; + return env->package_json_cache[path] = pjson; } enum ResolveExtensionsOptions { @@ -579,7 +579,8 @@ inline Maybe ResolveIndex(const URL& search) { Maybe ResolveMain(Environment* env, const URL& search) { URL pkg("package.json", &search); - PackageConfig pjson = GetPackageConfig(env, pkg.ToFilePath()); + const Environment::PackageConfig& pjson = + GetPackageConfig(env, pkg.ToFilePath()); if (!pjson.exists || !pjson.has_main) { return Nothing(); } @@ -596,7 +597,8 @@ Maybe ResolveModule(Environment* env, URL dir(""); do { dir = parent; - Maybe check = Resolve(env, "./node_modules/" + specifier, dir, true); + Maybe check = + Resolve(env, "./node_modules/" + specifier, dir, IgnoreMain); if (!check.IsNothing()) { const size_t limit = specifier.find('/'); const size_t spec_len = @@ -618,7 +620,7 @@ Maybe ResolveModule(Environment* env, Maybe ResolveDirectory(Environment* env, const URL& search, - bool check_pjson_main) { + package_main_check check_pjson_main) { if (check_pjson_main) { Maybe main = ResolveMain(env, search); if (!main.IsNothing()) @@ -632,7 +634,7 @@ Maybe ResolveDirectory(Environment* env, Maybe Resolve(Environment* env, const std::string& specifier, const URL& base, - bool check_pjson_main) { + package_main_check check_pjson_main) { URL pure_url(specifier); if (!(pure_url.flags() & URL_FLAGS_FAILED)) { // just check existence, without altering diff --git a/src/module_wrap.h b/src/module_wrap.h index 95e16abf6db85a..07abcda6d7e6b2 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -12,16 +12,15 @@ namespace node { namespace loader { -struct PackageConfig { - bool exists; - bool has_main; - std::string main; +enum package_main_check : bool { + CheckMain = true, + IgnoreMain = false }; v8::Maybe Resolve(Environment* env, const std::string& specifier, const url::URL& base, - bool read_pkg_json = false); + package_main_check read_pkg_json = CheckMain); class ModuleWrap : public BaseObject { public: