src: remove calls to deprecated v8 functions (NewFromUtf8) #21926

ryzokuken · 2018-07-21T16:59:02Z

Remove all calls to deprecated v8 functions (here: String::NewFromUtf8) inside
the code (src directory only).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Files

nodejs-github-bot · 2018-07-21T16:59:03Z

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/329/pipeline

ryzokuken · 2018-07-21T17:00:30Z

/cc @hashseed

Remove all calls to deprecated v8 functions (here: String::NewFromUtf8) inside the code (src directory only).

TimothyGu · 2018-07-21T18:17:33Z

src/exceptions.cc

 static Local<String> StringFromPath(Isolate* isolate, const char* path) {
 #ifdef _WIN32
  if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
+    v8::NewStringType type = v8::NewStringType::kNormal;


Doesn’t look like this is in the right place. There are two more instances of NewFromUtf8 below.

Ooh, this one was unfinished. Thanks, will change this.

I think this is unused now? Either way, I think it makes sense to keep these inline in the code (i.e. not create separate variables)

Okay, will make appropriate changes.

addaleax · 2018-07-21T18:58:13Z

src/exceptions.cc

  }
  Local<String> message = OneByteString(env->isolate(), msg);

+  v8::NewStringType type = v8::NewStringType::kNormal;


If you do decide to create new variables for these, can you name the more expressively (e.g. new_string_type or maybe string_type)?

addaleax

Looks good :)

addaleax · 2018-07-21T20:31:09Z

src/node.h

      v8::FunctionTemplate::New(isolate, callback, v8::Local<v8::Value>(), s);
-  v8::Local<v8::String> fn_name = v8::String::NewFromUtf8(isolate, name);
+  v8::Local<v8::String> fn_name = v8::String::NewFromUtf8(isolate, name,
+      v8::NewStringType::kNormal).ToLocalChecked();


I think for everything in this header, kInternalized would make more sense as the string type.

addaleax · 2018-07-21T20:32:59Z

src/node.cc

+        v8::NewStringType::kNormal).ToLocalChecked();
  }
  process->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "execPath"),
               exec_path_value);


Since we expect process.execPath to be long-lived, using internalized strings might make more sense. (It probably doesn’t matter all that much in individual cases like these, but the sum of the work we’re saving the GC might matter a bit.)

addaleax · 2018-07-21T20:41:39Z

src/node_os.cc

+        v8::NewStringType::kNormal).ToLocalChecked();
 #else
    name = OneByteString(env->isolate(), raw_name);
 #endif


Side note: While it may be true that UNIX systems are somewhat encoding-agnostic here, it’s more than reasonable to assume UTF8 as the default as well. It’s what people will expect if they name the interface from any input that uses UTF-8, which should be the most frequent case by far these days.

I’d suggest either merging these directly in this PR, since you’re already touching the code, or leaving a TODO comment about it.

addaleax · 2018-07-21T20:42:35Z

src/spawn_sync.cc

    js_result->Set(context, env()->signal_string(),
                   String::NewFromUtf8(env()->isolate(),
-                                       signo_string(term_signal_))).FromJust();
+                                       signo_string(term_signal_), v8::NewStringType::kNormal)


Does this hit the 80 char limit?

It does, dunno how autoformatting left this one out. Will do.

Use v8::NewStringType::kInternalized instead of v8::NewStringType::kNormal in different calls to v8::String::NewFromUtf8 in node.h and node.cc wherever it makes sense, to save the GC some work which would largely be redundant.

Use a UTF-8 encoded string for naming interfaces in unix, instead of using a binary encoding-agnostic string as had been done on windows previously.

ryzokuken · 2018-07-21T22:56:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15966/

ryzokuken · 2018-07-22T04:44:17Z

CI passed! Will be landing this once the 72 hours period passes.

addaleax · 2018-07-29T15:06:31Z

Landed in 51812ff...07cb697

Did a minor update to the commit message in 07cb697 (the previous behaviour was not encoding-agnostic). Hope that’s okay!

Remove all calls to deprecated v8 functions (here: String::NewFromUtf8) inside the code (src directory only). PR-URL: #21926 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

Use v8::NewStringType::kInternalized instead of v8::NewStringType::kNormal in different calls to v8::String::NewFromUtf8 in node.h and node.cc wherever it makes sense, to save the GC some work which would largely be redundant. PR-URL: #21926 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

Use a UTF-8 encoded string for naming interfaces in unix, instead of using Latin-1, as had been done on windows previously. PR-URL: #21926 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

ryzokuken · 2018-07-29T15:09:42Z

It totally is, thanks for landing this!

Remove all calls to deprecated v8 functions (here: String::NewFromUtf8) inside the code (src directory only). PR-URL: #21926 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

Use v8::NewStringType::kInternalized instead of v8::NewStringType::kNormal in different calls to v8::String::NewFromUtf8 in node.h and node.cc wherever it makes sense, to save the GC some work which would largely be redundant. PR-URL: #21926 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

Use a UTF-8 encoded string for naming interfaces in unix, instead of using Latin-1, as had been done on windows previously. PR-URL: #21926 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

hashseed

Generally, you want to use kInternalized for strings that are used as identifiers, e.g. variable names or property names.

Sorry for being late with this :)

hashseed · 2018-07-31T08:39:51Z

src/node.cc

  Local<Object> exports = Object::New(env->isolate());
-  Local<String> exports_prop = String::NewFromUtf8(env->isolate(), "exports");
+  Local<String> exports_prop = String::NewFromUtf8(env->isolate(), "exports",
+      v8::NewStringType::kNormal).ToLocalChecked();


You could argue that this should be kInternalized.

hashseed · 2018-07-31T08:40:27Z

src/node.cc

                      "_eval",
-                      String::NewFromUtf8(env->isolate(), eval_string));
+                      String::NewFromUtf8(env->isolate(), eval_string,
+                          v8::NewStringType::kNormal).ToLocalChecked());


Also kInternalized here.

hashseed · 2018-07-31T08:41:07Z

src/node.cc

                                          exec_path,
-                                          String::kNormalString,
-                                          exec_path_len);
+                                          v8::NewStringType::kInternalized,


Here on the other hand I don't know why this needs to be internalized.

hashseed · 2018-07-31T08:43:02Z

src/node_perf.cc

                                             entry.name().c_str(),
-                                             String::kNormalString),
-                         attr).FromJust();
+                                             v8::NewStringType::kNormal)


kInternalized

hashseed · 2018-07-31T08:43:08Z

src/node_perf.cc

-                         attr).FromJust();
+                                             v8::NewStringType::kNormal)
+                             .ToLocalChecked(),
+                         attr)


ryzokuken changed the title ~~[WIP] v8: remove calls to deprecated v8 functions (NewFromUtf8)~~ [WIP] src: remove calls to deprecated v8 functions (NewFromUtf8) Jul 21, 2018

src: remove calls to deprecated v8 functions (NewFromUtf8)

b729e20

Remove all calls to deprecated v8 functions (here: String::NewFromUtf8) inside the code (src directory only).

ryzokuken force-pushed the v8-deprecation-1 branch from 2b70708 to b729e20 Compare July 21, 2018 17:02

TimothyGu reviewed Jul 21, 2018

View reviewed changes

ryzokuken added 2 commits July 22, 2018 00:13

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

7bf4efe

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

c2f5a1b

addaleax reviewed Jul 21, 2018

View reviewed changes

ryzokuken added 6 commits July 22, 2018 00:29

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

025bffb

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

76f5004

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

1264ad8

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

d4a87e0

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

c7caef8

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

5b6232e

addaleax approved these changes Jul 21, 2018

View reviewed changes

ryzokuken added 3 commits July 22, 2018 03:28

src: use kInternalized instead of kNormal

560e336

Use v8::NewStringType::kInternalized instead of v8::NewStringType::kNormal in different calls to v8::String::NewFromUtf8 in node.h and node.cc wherever it makes sense, to save the GC some work which would largely be redundant.

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

5a0ef1d

src: use UTF-8 for naming interfaces in unix

f4bc20b

Use a UTF-8 encoded string for naming interfaces in unix, instead of using a binary encoding-agnostic string as had been done on windows previously.

ryzokuken changed the title ~~[WIP] src: remove calls to deprecated v8 functions (NewFromUtf8)~~ src: remove calls to deprecated v8 functions (NewFromUtf8) Jul 21, 2018

addaleax approved these changes Jul 21, 2018

View reviewed changes

fixup! src: remove calls to deprecated v8 functions (NewFromUtf8)

1ab48ad

tniessen approved these changes Jul 22, 2018

View reviewed changes

apapirovski approved these changes Jul 22, 2018

View reviewed changes

jasnell approved these changes Jul 23, 2018

View reviewed changes

addaleax closed this Jul 29, 2018

targos mentioned this pull request Jul 31, 2018

v10.8.0 proposal #22040

Merged

hashseed reviewed Jul 31, 2018

View reviewed changes

Uh oh!

src: remove calls to deprecated v8 functions (NewFromUtf8) #21926

src: remove calls to deprecated v8 functions (NewFromUtf8) #21926

Uh oh!

Conversation

ryzokuken commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Files

Uh oh!

nodejs-github-bot commented Jul 21, 2018

Uh oh!

ryzokuken commented Jul 21, 2018

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!

addaleax left a comment

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!

ryzokuken commented Jul 21, 2018

Uh oh!

ryzokuken commented Jul 22, 2018

Uh oh!

addaleax commented Jul 29, 2018

Uh oh!

ryzokuken commented Jul 29, 2018

Uh oh!

hashseed left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ryzokuken commented Jul 21, 2018 •

edited

Loading

hashseed left a comment •

edited

Loading