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.

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

Fix for bug #86 #87

Closed
wants to merge 1 commit into from
Closed

Fix for bug #86 #87

wants to merge 1 commit into from

Conversation

victor-cr
Copy link

@victor-cr victor-cr commented Dec 21, 2018

Fix for bug #86
Tests v8 before fix:

Excluding skipped: 89.5% (3630/4054) passed
Including skipped: 82.7% (3630/4392) passed

After fix:

Excluding skipped: 89.6% (3631/4054) passed
Including skipped: 82.7% (3631/4392) passed

Also, after the change the output equals to the one from v8 node.

@graalvmbot
Copy link
Collaborator

Hello Viktor Polishchuk, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address viktorpo -(at)- wix -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@victor-cr
Copy link
Author

victor-cr commented Dec 21, 2018

I have already signed OCA with my backup mail: my user name is victor.polischuk

@wirthi
Copy link
Member

wirthi commented Dec 21, 2018

Hi,

thanks for your contribution. I see you listed as Victor Polischuk - OpenJDK - victor.polischuk. I'll update our CI so it accepts this.

Best, Christian

@iamstolis
Copy link
Member

Thank you for an attempt to provide a fix for bug #86. Your patch really ensures that the expression mentioned in the bug does not trigger a SyntaxError. Unfortunately, it also results in other expressions (like Object({a=1})) not to trigger SyntaxError incorrectly.

@graalvmbot
Copy link
Collaborator

Viktor Polishchuk has signed the Oracle Contributor Agreement (based on email address viktorpo -(at)- wix -(dot)- com) so can contribute to this repository.

@victor-cr
Copy link
Author

Excluding skipped: 89.6% (3631/4054) passed
Including skipped: 82.7% (3631/4392) passed

Also I believe it covers only the case with async arrow functions

@iamstolis
Copy link
Member

Thank you for another attempt to fix bug #86. This attempt looks much better than the first one and is very close to a correct fix, I believe. Unfortunately, there are still some corner cases that are not handled correctly. async token can either represent a modifier of an async function or it can be an identifier (of a normal/non-async function possibly). Your fix does not handle the latter case correctly. It accepts expressions that should lead to SyntaxError. For example, async({a=1}) should trigger a SyntaxError but it does not.

In fact, I already have a fix for bug #86 ready for a review in our internal system. The fix handles the mentioned corner case correctly but is very similar to your fix otherwise.

Note that we appreciate external contributions. I am sorry that our efforts clashed on fixing of bug #86. If I knew that you will attempt to fix the issue on your own then I would not start the work on the fix myself.

@victor-cr
Copy link
Author

No problem, I was just blocked with the bug. I am glad it is fixed. Thank you.

@victor-cr victor-cr closed this Dec 22, 2018
graalvmbot pushed a commit that referenced this pull request Sep 1, 2021
Original commit message:

    M86-LTS: [compiler] Fix off-by-one error in kAdditiveSafeInteger

    (cherry picked from commit 798fbcb0a3e5a292fb775c37c19d9fe73bbac17c)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1198705
    Change-Id: I6b3ad82754e1ca72701ce57f16c4f085f8c87f77
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2835705
    Auto-Submit: Georg Neis <[email protected]>
    Commit-Queue: Nico Hartmann <[email protected]>
    Reviewed-by: Nico Hartmann <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#74033}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2850708
    Commit-Queue: Artem Sumaneev <[email protected]>
    Reviewed-by: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#87}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@0024503

PR-URL: nodejs/node#38481
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants