★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/30713Nouvelle recherche | Portail wanayoo
Skip to content

/ node

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.

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

deps: V8: cherry-pick 0dfd9ea51241 #30713

Closed
wants to merge 1 commit into from
Closed

deps: V8: cherry-pick 0dfd9ea51241 #30713

wants to merge 1 commit into from

Conversation

@bcoe
Copy link
Member

bcoe commented Nov 29, 2019

Original commit message:

[coverage] Fix coverage with default arguments

In the presence of default arguments, the body of the function gets
wrapped into another block. This caused our trailing-range-after-return
optimization to not apply, because the wrapper block had no source
range assigned. This CL correctly assignes a source range to that block,
which allows already present code to handle it correctly.

Note that this is not a real coverage bug; we've just been reporting
whitespace as uncovered. We're fixing it for consistency.

Originally reported on github.com/bcoe/c8/issues/66

Bug: v8:9952
Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

This addresses: bcoe/c8#66, bcoe/c8#151
@targos
targos approved these changes Nov 29, 2019
View changes
@cjihrig
cjihrig approved these changes Nov 29, 2019
View changes
@gengjiawen
gengjiawen approved these changes Nov 29, 2019
View changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
Copy link

nodejs-github-bot commented Nov 30, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27202/
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
Copy link

nodejs-github-bot commented Nov 30, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27203/
@nodejs nodejs deleted a comment from nodejs-github-bot Nov 30, 2019
@bcoe
deps: V8: cherry-pick 0dfd9ea51241
Verified
This commit was signed with a verified signature.
bcoe Benjamin E. Coe
GPG key ID: 84D97FAF3C07DF69 Learn about signing commits
Loading status checks…
0c43319 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea
@bcoe bcoe force-pushed the bcoe:fix-coverage-bug branch to 0c43319 Nov 30, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

nodejs-github-bot commented Nov 30, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27205/
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Nov 30, 2019

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2667/
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

nodejs-github-bot commented Nov 30, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27211/
@bcoe bcoe added the fast-track label Nov 30, 2019
@bcoe

This comment has been minimized.

Sign in to view
Copy link
Member Author

bcoe commented Nov 30, 2019

@addaleax it looks like the pertinent V8 tests (coverage-block-async, and coverage-block) have passed 👍 (it looks like unrelated V8 failures have been failing for all builds recently).

I would like to fast-track landing this patch, while I have some cycles today.

👍 or 👎
@Trott
Trott approved these changes Nov 30, 2019
View changes
bcoe added a commit that referenced this pull request Nov 30, 2019
@bcoe
deps: V8: cherry-pick 0dfd9ea51241
Verified
This commit was signed with a verified signature.
bcoe Benjamin E. Coe
GPG key ID: 84D97FAF3C07DF69 Learn about signing commits
Loading status checks…
5961161 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

PR-URL: #30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bcoe

This comment has been minimized.

Sign in to view
Copy link
Member Author

bcoe commented Nov 30, 2019

Landed in 5961161
@bcoe bcoe closed this Nov 30, 2019
@bcoe bcoe deleted the bcoe:fix-coverage-bug branch Nov 30, 2019
targos added a commit that referenced this pull request Dec 1, 2019
@bcoe @targos
deps: V8: cherry-pick 0dfd9ea51241
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
0ba877a 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

PR-URL: #30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
Merged
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Jan 14, 2020

Should this be backported to v12.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.
@bcoe

This comment has been minimized.

Sign in to view
Copy link
Member Author

bcoe commented Jan 14, 2020

@targos ideally we'd backport this, sounds like I should go through that guide?
@bcoe bcoe mentioned this pull request Jan 19, 2020
Closed
3 of 4 tasks complete
bcoe added a commit to bcoe/node-1 that referenced this pull request Jan 19, 2020
@bcoe
deps: V8: cherry-pick 0dfd9ea51241
dd8c833 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

PR-URL: nodejs#30713
Backport-PR-URL: nodejs#31412
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 30, 2020
@bcoe @MylesBorins
deps: V8: cherry-pick 0dfd9ea51241
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
02d7283 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

Backport-PR-URL: #31412
PR-URL: #30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jan 30, 2020
Closed
BethGriggs added a commit that referenced this pull request Feb 6, 2020
@bcoe @BethGriggs
deps: V8: cherry-pick 0dfd9ea51241
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
e2cd110 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

Backport-PR-URL: #31412
PR-URL: #30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
@bcoe Michael Bashurov
deps: V8: cherry-pick 0dfd9ea51241
94ceb84 
Original commit message:

    [coverage] Fix coverage with default arguments

    In the presence of default arguments, the body of the function gets
    wrapped into another block. This caused our trailing-range-after-return
    optimization to not apply, because the wrapper block had no source
    range assigned. This CL correctly assignes a source range to that block,
    which allows already present code to handle it correctly.

    Note that this is not a real coverage bug; we've just been reporting
    whitespace as uncovered. We're fixing it for consistency.

    Originally reported on github.com/bcoe/c8/issues/66

    Bug: v8:9952
    Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
    Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

PR-URL: nodejs#30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott

@targos targos

@cjihrig cjihrig

@gengjiawen gengjiawen

Assignees
No one assigned
Labels
V8 Engine author ready backported-to-v12.x build fast-track
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@bcoe @nodejs-github-bot @addaleax @targos @Trott @cjihrig @gengjiawen @MylesBorins
You can’t perform that action at this time.