src,deps: add isolate parameter to String::Concat by targos · Pull Request #22521 · nodejs/node

targos · 2018-08-25T11:47:55Z

Partially backport an upstream commit that deprecates String::Concat
without the isolate parameter. This overload has already been removed
in V8 7.0.

Refs: v8/v8@8a011b5

/cc @nodejs/v8-update

I'd like your opinion on this proactive approach about V8 deprecations. If everyone is fine with it, I'm ready to open other PRs for StackTrace::GetFrame, String::Write, etc.
I would like to do this because it can be backported to v10.x.

Checklist

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

Partially backport an upstream commit that deprecates String::Concat without the isolate parameter. This overload has already been removed in V8 7.0. Refs: v8/v8@8a011b5

nodejs-github-bot · 2018-08-25T11:47:56Z

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

BridgeAR · 2018-08-25T11:58:22Z

@targos I think it's actually a good idea to be proactive about it. Having dealt with the issue early on always sounds good to me.

ryzokuken

LGTM overall, with a single comment down below and a minor concern:

The signature of WinapiErrnoException and ErrnoException aren't changed, so the isolate arg had been there all along, right?
Why did we just shift from env()->isolate to isolate then? (Like, I agree that we should, but shouldn't it have been done already?)

ryzokuken · 2018-08-25T12:37:46Z

deps/v8/src/api.cc

 }

+Local<String> v8::String::Concat(Local<String> left, Local<String> right) {
+  i::Handle<i::String> left_string = Utils::OpenHandle(*left);


A bit hacky (opening a handle just to fetch the isolate), but idk if there's a better way. @addaleax do you?

Perhaps one could get the current env and extract the associated isolate? Idk how different the result would be, but it's certainly less hacky IMO.

That's just copied from the V8 source code (see the Refs link)

I get it. It works so it shouldn't be a problem anyway.

targos · 2018-08-25T12:47:53Z

The signature of WinapiErrnoException and ErrnoException aren't changed, so the isolate arg had been there all along, right?
Why did we just shift from env()->isolate to isolate then? (Like, I agree that we should, but shouldn't it have been done already?)

Yes, WinapiErrnoException and ErrnoException already receive the isolate as a parameter. Unless I'm missing something, we can and should be using it instead of getting the pointer from the environment.

ryzokuken · 2018-08-25T12:50:25Z

As I said, I agree that we should just directly use it, I just had been wondering why it wasn't being used before this patch if the parameter always existed.

refack · 2018-08-25T20:54:48Z

IMHO it's better to review, and to track if this is done in two commits. Quick look at the code seems like it's possible.

targos · 2018-08-26T07:14:24Z

@refack yeah, sorry. I thought the changeset was small enough to not be a problem. I'll do multiple commits for the next PR.

targos · 2018-08-26T07:14:55Z

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

targos · 2018-08-28T12:13:01Z

Landed in 08aad66

Partially backport an upstream commit that deprecates String::Concat without the isolate parameter. This overload has already been removed in V8 7.0. PR-URL: #22521 Refs: v8/v8@8a011b5 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

addaleax · 2018-08-28T20:18:54Z

@targos This would need a backport for v10.x :)

targos · 2018-08-28T20:22:30Z

Oops, looks like we are working on the same thing :)
It's normal that the embedder string is off by one in v10.x compared to master. There is one commit that could not be backported because of ABI compatibility.

addaleax · 2018-08-28T20:25:59Z

@targos If you were on IRC I’d have asked you whether you’re going to do a 10.x this week. 😄

I haven’t done any backporting attempts myself, but if the embedder string is one off, would it make sense to just bump it to the master level?

targos · 2018-08-28T20:29:04Z

@addaleax I'm on IRC now. I'm not really preparing the release btw, just keeping the branch up-to-date.

if the embedder string is one off, would it make sense to just bump it to the master level?

Good idea. I'll open a PR to do that.

Partially backport an upstream commit that deprecates String::Concat without the isolate parameter. This overload has already been removed in V8 7.0. PR-URL: #22521 Refs: v8/v8@8a011b5 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

src,deps: add isolate parameter to String::Concat

135dd4c

Partially backport an upstream commit that deprecates String::Concat without the isolate parameter. This overload has already been removed in V8 7.0. Refs: v8/v8@8a011b5

targos requested review from addaleax and ryzokuken August 25, 2018 11:47

nodejs-github-bot 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 Aug 25, 2018

ryzokuken approved these changes Aug 25, 2018

View reviewed changes

devsnek approved these changes Aug 25, 2018

View reviewed changes

TimothyGu approved these changes Aug 25, 2018

View reviewed changes

refack approved these changes Aug 25, 2018

View reviewed changes

targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 26, 2018

targos mentioned this pull request Aug 26, 2018

src,deps: backport and use non-deprecated V8 methods with isolate #22531

Closed

2 tasks

targos closed this Aug 28, 2018

targos deleted the v8-isolate-concat branch August 28, 2018 12:13

targos added v8 engine Issues and PRs related to the V8 dependency. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 28, 2018

addaleax added the backport-requested-v10.x label Aug 28, 2018

targos added dont-land-on-v6.x and removed backport-requested-v10.x labels Aug 28, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

Uh oh!

Conversation

targos commented Aug 25, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 25, 2018

Uh oh!

BridgeAR commented Aug 25, 2018

Uh oh!

ryzokuken 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!

ryzokuken Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

targos Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

ryzokuken Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

targos commented Aug 25, 2018

Uh oh!

ryzokuken commented Aug 25, 2018

Uh oh!

refack commented Aug 25, 2018

Uh oh!

targos commented Aug 26, 2018

Uh oh!

targos commented Aug 26, 2018

Uh oh!

targos commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Aug 28, 2018

Uh oh!

targos commented Aug 28, 2018

Uh oh!

addaleax commented Aug 28, 2018

Uh oh!

targos commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ryzokuken left a comment •

edited

Loading

targos commented Aug 28, 2018 •

edited

Loading