Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cherry pick 3c39bac from upstream V8 #9138

Closed
wants to merge 1 commit into from
Closed

Cherry pick 3c39bac from upstream V8 #9138

wants to merge 1 commit into from

Conversation

cristiancavalli
Copy link

@cristiancavalli cristiancavalli commented Oct 17, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

Backport of bugfix from upstream V8

cc @nodejs/v8

@nodejs-github-bot nodejs-github-bot added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Oct 17, 2016
@ofrobots
Copy link
Contributor

ofrobots commented Oct 17, 2016 •
edited
Loading

For convenience, link to V8 commit: v8/v8@3c39bac.

CI: https://ci.nodejs.org/job/node-test-pull-request/4545/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/362/

EDIT: fix link.

@MylesBorins
Copy link
Contributor

should this land on v4.x?

@ofrobots
Copy link
Contributor

Bug in destructuring, which isn't in v4.x, so this shouldn't be needed on v4.x.

/cc @nodejs/v8 as I don't think the original mention worked.

targos
targos approved these changes Oct 17, 2016
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM

@rvagg
Copy link
Member

rvagg commented Oct 18, 2016

this isn't going to make it in to 6.9.0 unfortunately, next release though

@jasnell
Copy link
Member

jasnell commented Oct 19, 2016

actually, I take that back, it looks like this needs to be rebased and updated.

@cristiancavalli
deps: cherry-pick 3c39bac from V8 upstream
17e2b7f 
Original Commit Message:
  Don't skip hole checks inside patterns in parameter lists

  Previously, b6e9f625c17f3a688139426771e2cb34fbdcb46e fixed self-assignment
  in parameters to throw. But it failed to deal with the case of
  destructuring with defaults. This patch extends that previous approach
  to always treat the end of a parameter as its initializer position,
  whether it has an initializer or not.

  This is the minimal change to make it easy to merge; a follow-up
  will rename the field of Parameter from "initializer_end_position"
  to "end_position".

  BUG=v8:5454
  Review-Url: https://codereview/chromium.org/2390943002
  Cr-Commit-Position: refs/heads/master@{nodejs#39962}
@ofrobots
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/4851/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/418/

@ofrobots
Copy link
Contributor

With the exception of suspected flaky timeouts on freebsd, the CI is looking green. Will land soon.

@ofrobots
Copy link
Contributor

@jasnell Since this was rebased and updated I am assuming that your LGTM applies again.

ofrobots pushed a commit that referenced this pull request Nov 16, 2016
@ofrobots
deps: cherry-pick 3c39bac from V8 upstream
a8840bb 
Original commit message:
  Don't skip hole checks inside patterns in parameter lists

  Previously, b6e9f625c17f3a688139426771e2cb34fbdcb46e fixed self-assignment
  in parameters to throw. But it failed to deal with the case of
  destructuring with defaults. This patch extends that previous approach
  to always treat the end of a parameter as its initializer position,
  whether it has an initializer or not.

  This is the minimal change to make it easy to merge; a follow-up
  will rename the field of Parameter from "initializer_end_position"
  to "end_position".

  BUG=v8:5454
  Review-Url: https://codereview/chromium.org/2390943002
  Cr-Commit-Position: refs/heads/master@{#39962}

PR-URL: #9138
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
@ofrobots
Copy link
Contributor

Landed on v6.x-staging as a8840bb.

@cristiancavalli It seems that after the rebase the V8 version bump got dropped. I edited the commit at landing time to add it back.

@ofrobots ofrobots closed this Nov 16, 2016
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Dec 6, 2016
@MylesBorins
2016-12-06, Version 6.9.2 'Boron' (LTS) Release
4e11cae 
This LTS release comes with 144 commits. This includes 47 that are docs
related, 46 that are test related, 15 which are build / tools related,
and 9 commits which are updates to dependencies

Notable Changes:

* buffer:
  - coerce slice parameters consistently
    (Sakthipriyan Vairamani (thefourtheye))
    #9101
* deps:
    - *npm*:
      - upgrade npm to 3.10.9 (Kat Marchán)
       #9286
    - *V8*:
      - Various fixes to destructuring edge cases
        - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli)
          #9138
        - cherry pick 7166503 from upstream v8 (Cristian Cavalli)
          #9173
* gtest:
  - the test reporter now outputs tap comments as yamlish
    (Johan Bergström) #9262
* inspector:
  - inspector now prompts user to use 127.0.0.1 rather than localhost
    (Eugene Ostroukhov) #9451
* tls:
  - fix memory leak when writing data to TLSWrap instance during
    handshake (Fedor Indutny)
    #9586

PR-URL: #9735
MylesBorins pushed a commit that referenced this pull request Dec 6, 2016
@MylesBorins
2016-12-06, Version 6.9.2 'Boron' (LTS) Release
94a3aef 
This LTS release comes with 144 commits. This includes 47 that are docs
related, 46 that are test related, 15 which are build / tools related,
and 9 commits which are updates to dependencies

Notable Changes:

* buffer:
  - coerce slice parameters consistently
    (Sakthipriyan Vairamani (thefourtheye))
    #9101
* deps:
    - *npm*:
      - upgrade npm to 3.10.9 (Kat Marchán)
       #9286
    - *V8*:
      - Various fixes to destructuring edge cases
        - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli)
          #9138
        - cherry pick 7166503 from upstream v8 (Cristian Cavalli)
          #9173
* gtest:
  - the test reporter now outputs tap comments as yamlish
    (Johan Bergström) #9262
* inspector:
  - inspector now prompts user to use 127.0.0.1 rather than localhost
    (Eugene Ostroukhov) #9451
* tls:
  - fix memory leak when writing data to TLSWrap instance during
    handshake (Fedor Indutny)
    #9586

PR-URL: #9735
MylesBorins pushed a commit that referenced this pull request Dec 6, 2016
@MylesBorins
2016-12-06, Version 6.9.2 'Boron' (LTS) Release
c4391f4 
This LTS release comes with 144 commits. This includes 47 that are docs
related, 46 that are test related, 15 which are build / tools related,
and 9 commits which are updates to dependencies

Notable Changes:

* buffer:
  - coerce slice parameters consistently
    (Sakthipriyan Vairamani (thefourtheye))
    #9101
* deps:
    - *npm*:
      - upgrade npm to 3.10.9 (Kat Marchán)
       #9286
    - *V8*:
      - Various fixes to destructuring edge cases
        - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli)
          #9138
        - cherry pick 7166503 from upstream v8 (Cristian Cavalli)
          #9173
* gtest:
  - the test reporter now outputs tap comments as yamlish
    (Johan Bergström) #9262
* inspector:
  - inspector now prompts user to use 127.0.0.1 rather than localhost
    (Eugene Ostroukhov) #9451
* tls:
  - fix memory leak when writing data to TLSWrap instance during
    handshake (Fedor Indutny)
    #9586

PR-URL: #9735
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016
@imyller
2016-12-06, Version 6.9.2 'Boron' (LTS) Release
33c85e7 
    This LTS release comes with 144 commits. This includes 47 that are docs
    related, 46 that are test related, 15 which are build / tools related,
    and 9 commits which are updates to dependencies

    Notable Changes:

    * buffer:
      - coerce slice parameters consistently
        (Sakthipriyan Vairamani (thefourtheye))
        nodejs/node#9101
    * deps:
        - *npm*:
          - upgrade npm to 3.10.9 (Kat Marchan)
           nodejs/node#9286
        - *V8*:
          - Various fixes to destructuring edge cases
            - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli)
              nodejs/node#9138
            - cherry pick 7166503 from upstream v8 (Cristian Cavalli)
              nodejs/node#9173
    * gtest:
      - the test reporter now outputs tap comments as yamlish
        (Johan Bergstrom) nodejs/node#9262
    * inspector:
      - inspector now prompts user to use 127.0.0.1 rather than localhost
        (Eugene Ostroukhov) nodejs/node#9451
    * tls:
      - fix memory leak when writing data to TLSWrap instance during
        handshake (Fedor Indutny)
        nodejs/node#9586

    PR-URL: nodejs/node#9735

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016
@imyller
2016-12-06, Version 6.9.2 'Boron' (LTS) Release
0ff3916 
    This LTS release comes with 144 commits. This includes 47 that are docs
    related, 46 that are test related, 15 which are build / tools related,
    and 9 commits which are updates to dependencies

    Notable Changes:

    * buffer:
      - coerce slice parameters consistently
        (Sakthipriyan Vairamani (thefourtheye))
        nodejs/node#9101
    * deps:
        - *npm*:
          - upgrade npm to 3.10.9 (Kat Marchan)
           nodejs/node#9286
        - *V8*:
          - Various fixes to destructuring edge cases
            - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli)
              nodejs/node#9138
            - cherry pick 7166503 from upstream v8 (Cristian Cavalli)
              nodejs/node#9173
    * gtest:
      - the test reporter now outputs tap comments as yamlish
        (Johan Bergstrom) nodejs/node#9262
    * inspector:
      - inspector now prompts user to use 127.0.0.1 rather than localhost
        (Eugene Ostroukhov) nodejs/node#9451
    * tls:
      - fix memory leak when writing data to TLSWrap instance during
        handshake (Fedor Indutny)
        nodejs/node#9586

    PR-URL: nodejs/node#9735

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
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.
8 participants
@cristiancavalli @ofrobots @MylesBorins @rvagg @jasnell @bnoordhuis @targos @nodejs-github-bot