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

deps,util: V8: backport 74571c8 #25941

Closed
wants to merge 2 commits into from
Closed

deps,util: V8: backport 74571c8 #25941

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 5, 2019

Fixes: #24629

Please have a look at the commit messages for further details.

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

@BridgeAR BridgeAR requested a review from targos February 5, 2019 03:25
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 5, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 5, 2019 •
edited
Loading

CI https://ci.nodejs.org/job/node-test-pull-request/20587/
V8-CI https://ci.nodejs.org/job/node-test-commit-v8-linux/2077/ ✔️

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 5, 2019

@nodejs/v8-update PTAL

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 7, 2019 •
edited
Loading

Resumed CI https://ci.nodejs.org/job/node-test-commit/25687/ ✔️

@BridgeAR
deps: V8: backport 74571c8
0a0ef6e 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: nodejs#24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8
@BridgeAR
util: update set iterator entries inspection
305201b 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.
@BridgeAR
Copy link
Member Author

Rebased due to a conflict in the v8_embedder_string. Both CIs were green earlier and there was no other change, so I guess it should be fine without running new CIs.

It would be great to get another LG @nodejs/v8-update

pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Feb 20, 2019
@BridgeAR
deps: V8: backport 74571c8
e557647 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: nodejs#24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8

PR-URL: nodejs#25941
Fixes: nodejs#24629
Reviewed-By: Michaël Zasso <[email protected]>
pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Feb 20, 2019
@BridgeAR
util: update set iterator entries inspection
3d62d0c 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.

PR-URL: nodejs#25941
Fixes: nodejs#24629
Reviewed-By: Michaël Zasso <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in e557647 and 3d62d0c.

@BridgeAR BridgeAR closed this Feb 20, 2019
addaleax pushed a commit that referenced this pull request Feb 21, 2019
@BridgeAR @addaleax
deps: V8: backport 74571c8
a09fb30 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: #24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
@BridgeAR @addaleax
util: update set iterator entries inspection
f32477a 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@BridgeAR @rvagg
deps: V8: backport 74571c8
206e4b0 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: #24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@BridgeAR @rvagg
util: update set iterator entries inspection
4bf58ac 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
@BridgeAR BridgeAR deleted the backport-v8-fix branch January 20, 2020 11:53
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

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BridgeAR @nodejs-github-bot @targos