Skip to content

Conversation

@bnoordhuis
Copy link
Member

We (including yours truly) have been less than stellar with bumping
the patch level for every change to deps/v8. This commit rectifies
that and covers the following commits:

2bbee49 v8: fix build errors with g++ 7
f882f47 deps: cherry-pick 79aee39 from upstream v8
9f73df5 deps: cherry-pick 22858cb from V8 upstream
a735c16 deps: backport ec1ffe3 from upstream V8
6130d54 deps: backport 8dde6ac from upstream V8

This is a chery-pick if you consider reducing the context to -C2 a cherry-pick; WordIsSmi has been renamed to TaggedIsSmi upstream. Original commit message: [builtins] Fix pointer comparison in ToString builtin. This fixes the bogus{Word32Equal} comparison in the ToString builtin implementing Object.prototype.toString to be a pointer-size{WordEqual} comparison instead. Comparing just the lower half-word is insufficient on 64-bit architectures. [email protected] TEST=mjsunit/regress/regress-crbug-664506 BUG=chromium:664506 Review-Url: https://codereview.chromium.org/2496043003 Cr-Commit-Position: refs/heads/master@{nodejs#40963} Fixes: nodejs#12411 PR-URL: nodejs#12412 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This is a local patch because upstream fixed it differently by moving large chunks of code out of objects.h. We cannot easily back-port those changes due to their size and invasiveness. Fixes: nodejs#10388 PR-URL: nodejs#12392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]>
We (including yours truly) have been less than stellar with bumping the patch level for every change to deps/v8. This commit rectifies that and covers the following commits: 2bbee49 v8: fix build errors with g++ 7 f882f47 deps: cherry-pick 79aee39 from upstream v8 9f73df5 deps: cherry-pick 22858cb from V8 upstream a735c16 deps: backport ec1ffe3 from upstream V8 6130d54 deps: backport 8dde6ac from upstream V8
@nodejs-github-botnodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 20, 2017
@jasnell
Copy link
Member

I'm wondering if there's a way we can automate this somehow... such as having the github bot check for the patch level bump if the commit touches deps/v8 at all... or having a pre-push git hook that checks for it.

@bnoordhuis
Copy link
MemberAuthor

I've been thinking about that. A PR-time check would be annoying because version numbers tend to be conflict magnets. A push-time check would be workable most of the time but not when doing housekeeping; i.e., there should be an opt-out.

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@bnoordhuis
Copy link
MemberAuthor

I'm wondering if there's a way we can automate this somehow...

To be clear I wasn't disagreeing it's a good idea. Case in point: I forgot to bump the patch level when I merged #12460...

@evanlucasevanlucasforce-pushed the v7.x-staging branch 2 times, most recently from d4ceb59 to 69a8053CompareMay 3, 2017 12:56
@bnoordhuis
Copy link
MemberAuthor

I'll close this, it's out of date. On the topic of automation, maybe we should do the patch level bump at release time (and only when patches were landed, of course.) Good idea, bad idea?

@gibfahn
Copy link
Member

On the topic of automation, maybe we should do the patch level bump at release time (and only when patches were landed, of course.) Good idea, bad idea?

If it's still a manual process I suspect it'll still get forgotten during the release.

A PR-time check would be annoying because version numbers tend to be conflict magnets.

Maybe a lint rule? That way you could ignore it if you knew it wasn't correct (and you could rerun just before landing the commit to make sure it was okay).

@bnoordhuis
Copy link
MemberAuthor

Releasers follow a script, don't they? That said, a lint rule seems like a good idea.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@bnoordhuis@jasnell@gibfahn@danbev@mhdawson@nodejs-github-bot