FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: backport 224d376 from V8 upstream by jBarz · Pull Request #10526 · nodejs/node · GitHub

/ node Public

deps: backport 224d376 from V8 upstream - #10526

Closed
jBarz wants to merge 1 commit into
nodejs:masterfrom
jBarz:v8_new_delete
ConversationCommits (1)ChecksFiles changed
Closed

deps: backport 224d376 from V8 upstream#10526
jBarz wants to merge 1 commit into
nodejs:masterfrom
jBarz:v8_new_delete

Conversation

jBarz commented Dec 29, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Orignial commit message:
Abort in delete operators that shouldn't be called.

Section 3.2 of the C++ standard states that destructor
definitions implicitly "use" operator delete functions.
Therefore, these operator delete functions must be
defined even if they are never called by user code
explicitly.
http://www.open-std.org/JTC1/SC22/WG21/docs/
cwg_defects.html#261

gcc allows them to remain as empty definitions. However,
not all compilers allow this. (e.g. xlc on zOS). This pull
request creates definitions which if ever called, result
in an abort.

R=danno@chromium.org,jochen@chromium.org
BUG=
LOG=N

Review-Url: https://codereview.chromium.org/2588433002
Cr-Commit-Position: refs/heads/master@{#41981}

Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

v8

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Dec 29, 2016

jBarz commented Dec 29, 2016

Copy link
Copy Markdown
Contributor Author

cc @gibfahn

sam-github commented Dec 29, 2016

Copy link
Copy Markdown
Contributor

LGTM

Did you run into this as a problem on a non-gcc system?

jBarz commented Dec 29, 2016

Copy link
Copy Markdown
Contributor Author

I ran into this on z/OS which uses the xlc compiler.

gibfahn commented Dec 30, 2016

Copy link
Copy Markdown
Member

I think the V8 patch level needs to be updated. The process should be documented in the updating v8 guide.

mhdawson approved these changes Jan 4, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson commented Jan 4, 2017

Copy link
Copy Markdown
Member

CI runs: https://ci.nodejs.org/job/node-test-pull-request/5703/
https://ci.nodejs.org/job/node-test-commit-v8-linux/493/

jasnell commented Jan 6, 2017

Copy link
Copy Markdown
Member

Weird failure in the arm CI job. Appears unrelated but just in case: https://ci.nodejs.org/job/node-test-pull-request/5730/

jBarz commented Jan 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It is weird because when I click on the link for details, the test appears to have passed. :-)
Unless I am missing something

jasnell commented Jan 6, 2017

Copy link
Copy Markdown
Member

The results showing now are for the new run I just kicked off... which does appear to have passed

gibfahn commented Jan 6, 2017

Copy link
Copy Markdown
Member

The Github check sometimes wrongly reports that test/arm failed (nodejs/build#572), it's nothing to worry about.

gibfahn commented Jan 6, 2017

Copy link
Copy Markdown
Member

@jBarz do the changes in include/v8.h from v8/v8@224d37 not need to be included?

jBarz commented Jan 7, 2017

Copy link
Copy Markdown
Contributor Author

@gib: v8.h in the master branch in chromium underwent some changes that needed to be reversed.
But node has not absorbed those changes yet so this PR does not need them.

gibfahn approved these changes Jan 7, 2017
View reviewed changes

gibfahn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In that case LGTM (in that it matches the upstream commit). Should be reviewed by @nodejs/v8 and/or @MylesBorins though.

ofrobots approved these changes Jan 10, 2017
View reviewed changes

ofrobots left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM w/ nit: the commit abstract should say 'backport' as this is not a clean cherry-pick.

ofrobots commented Jan 18, 2017

Copy link
Copy Markdown
Contributor

Another nit: prefer 7-character commit ids.

deps: backport 224d376 from V8 upstream
99c7f8c 
Orignial commit message:
  Abort in delete operators that shouldn't be called.

  Section 3.2 of the C++ standard states that destructor
  definitions implicitly "use" operator delete functions.
  Therefore, these operator delete functions must be
  defined even if they are never called by user code
  explicitly.
  http://www.open-std.org/JTC1/SC22/WG21/docs/
  cwg_defects.html#261

  gcc allows them to remain as empty definitions. However,
  not all compilers allow this. (e.g. xlc on zOS). This pull
  request creates definitions which if ever called, result
  in an abort.

  R=danno@chromium.org,jochen@chromium.org
  BUG=
  LOG=N

  Review-Url: https://codereview.chromium.org/2588433002
  Cr-Commit-Position: refs/heads/master@{nodejs#41981}

jBarz commented Jan 18, 2017

Copy link
Copy Markdown
Contributor Author

addressed nits

jBarz changed the title deps: cherry-pick 224d37 from V8 upstream deps: cherry-pick 224d376 from V8 upstream Jan 18, 2017
jBarz changed the title deps: cherry-pick 224d376 from V8 upstream deps: backport 224d376 from V8 upstream Jan 18, 2017

ofrobots commented Jan 19, 2017

Copy link
Copy Markdown
Contributor

Thanks. LGTM.

bnoordhuis approved these changes Jan 19, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. @jBarz Did you request backports to 5.5 and 5.6?

jBarz commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

oh no :-(
I will do that

jBarz commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

I have requested backports to 5.5, 5.6.

targos commented Jan 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Does it need to be applied to v7, v6 or v4?
What is the status on the backport request?

jBarz commented Jan 28, 2017

Copy link
Copy Markdown
Contributor Author

This fix is required on v6.x and above.
The backport to node v6.x was successfull (#10546)
Backport to v8 5.5 was rejected (too late). Doesn't matter I think because of #10546
Backport to v8 5.6 was accepted.

jasnell commented Mar 24, 2017

Copy link
Copy Markdown
Member

Ping. any updates on this one?

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

gibfahn commented Mar 25, 2017

Copy link
Copy Markdown
Member

@jBarz if you could rebase and confirm this is still needed, we should get it landed, especially if it's already gone into v6.x

jBarz commented Mar 25, 2017

Copy link
Copy Markdown
Contributor Author

This backport is no longer needed because it is already backported via #11752

jBarz closed this Mar 25, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
+3 more reviewers
ofrobots ofrobots approved these changes
mhdawson mhdawson approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stalled Issues and PRs that are stalled. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL