Skip to content

src: only call FatalException if not verbose#12826

Closed
danbev wants to merge 3 commits intonodejs:masterfrom
danbev:is-verbose-todo
Closed

src: only call FatalException if not verbose#12826
danbev wants to merge 3 commits intonodejs:masterfrom
danbev:is-verbose-todo

Conversation

@danbev
Copy link
Copy Markdown
Contributor

@danbev danbev commented May 4, 2017

This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels May 4, 2017
@jasnell
Copy link
Copy Markdown
Member

jasnell commented May 4, 2017

@nodejs/v8
@bnoordhuis

cjihrig
cjihrig approved these changes May 4, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Second commit LGTM

@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 4, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/7849/

addaleax
addaleax approved these changes May 7, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

For my understanding: There isn’t any behavioural change because in the verbose case V8 will call OnMessage, right? If so, LGTM.

@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 9, 2017

For my understanding: There isn’t any behavioural change because in the verbose case V8 will call OnMessage, right? If so, LGTM.

Yes that is my understanding as well, if a TryCatch is used and set to verbose, the handler will be called.

bnoordhuis
bnoordhuis approved these changes May 15, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM if you bump the patch level in v8-version.h. Did you request a back-port to the 5.8 branch?

danbev added 3 commits May 15, 2017 14:20
@danbev
deps: cherry-pick f5fad6d from upstream v8
46f73c2 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{nodejs#45018}
@danbev
src: only call FatalException if not verbose
49abd1a 
This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.
@danbev
deps: bump V8_PATCH_LEVEL to 39
7af80be
@danbev danbev force-pushed the is-verbose-todo branch from 3162e34 to 7af80be Compare May 15, 2017 12:37
@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 15, 2017

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/8082/

@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 15, 2017

if you bump the patch level in v8-version.h

I've bumped the patch level now.

Did you request a back-port to the 5.8 branch?

I've not done so but would be happy to. Just to clarify, you mean create back-port this to 5.8 in V8 right? I've never done that before so if you have any pointers about how to go about doing that (what branch would I base the change set on)?

danbev added a commit to danbev/node that referenced this pull request May 16, 2017
@danbev
deps: cherry-pick f5fad6d from upstream v8
986e1d2 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{nodejs#45018}

PR-URL: nodejs#12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
danbev added a commit to danbev/node that referenced this pull request May 16, 2017
@danbev
src: only call FatalException if not verbose
fefab90 
This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

PR-URL: nodejs#12826
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 16, 2017

Landed in fefab90 and 986e1d2

@danbev danbev closed this May 16, 2017
@danbev danbev deleted the is-verbose-todo branch May 16, 2017 08:05
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented May 16, 2017

@danbev Just so you know, the Author tag is "daniel.bevenius" in commit 986e1d2. git am-d from upstream, I assume?

@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 17, 2017

@bnoordhuis Thanks, I forgot to change it. I'll see if I can change my name in V8 so they are the same just in case I do this again and forget.
While I have you there would you be able to comment about my question above regarding the backporting?

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented May 17, 2017

@danbev https://github.com/v8/v8/wiki/Merging-&-Patching#is-there-a-tldr

@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 17, 2017

@bnoordhuis perfect, thanks!

@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented May 18, 2017

V8 backport request

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@danbev
deps: cherry-pick f5fad6d from upstream v8
749a0e8 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{nodejs#45018}

PR-URL: nodejs#12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@danbev
src: only call FatalException if not verbose
ee1fcf9 
This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

PR-URL: nodejs#12826
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@danbev @addaleax
deps: cherry-pick f5fad6d from upstream v8
de7b668 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax mentioned this pull request Jun 17, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@danbev @addaleax
deps: cherry-pick f5fad6d from upstream v8
c52c757 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Jun 22, 2017

I'm a bit confused as to why 986e1d2 is showing up in both the v8.0.0 proposal as well as the v8.2.0 proposal. It would appear that only one of the commits from this PR ended up making its way to the 8.0.0 release which weird me out

/cc @jasnell @addaleax

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Jun 22, 2017

@MylesBorins The commit you linked was being re-applied as part of the V8 5.9 upgrade, but both were part of 8.0.0 … does that make sense?

addaleax pushed a commit that referenced this pull request Jun 24, 2017
@danbev @addaleax
deps: cherry-pick f5fad6d from upstream v8
5cd0db3 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
@danbev @rvagg
deps: cherry-pick f5fad6d from upstream v8
50c0d15 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@danbev @addaleax
deps: cherry-pick f5fad6d from upstream v8
068e03d 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Jul 17, 2017

Should this be backported to v6.x?

addaleax pushed a commit that referenced this pull request Jul 18, 2017
@danbev @addaleax
deps: cherry-pick f5fad6d from upstream v8
dd3bfd9 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit to targos/node that referenced this pull request Jul 21, 2017
@danbev @targos
deps: cherry-pick f5fad6d from upstream v8
ad7d46b 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{nodejs#45018}

PR-URL: nodejs#12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@danbev @addaleax
deps: cherry-pick f5fad6d from upstream v8
4c4f647 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Closed
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@danbev @MylesBorins
deps: cherry-pick f5fad6d from upstream v8
c7622e1 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@danbev @MylesBorins
src: only call FatalException if not verbose
2914926 
This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

PR-URL: #12826
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@danbev @MylesBorins
deps: cherry-pick f5fad6d from upstream v8
7fed989 
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{#45018}

PR-URL: #12826
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@danbev @MylesBorins
src: only call FatalException if not verbose
403c45f 
This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

PR-URL: #12826
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. 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.

7 participants

@danbev @jasnell @bnoordhuis @MylesBorins @addaleax @cjihrig @nodejs-github-bot