★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24365Nouvelle 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

src: emit warnings from V8 #24365

Merged
merged 1 commit into from Nov 16, 2018
+50 −7

Conversation

Reviewers

@bnoordhuis bnoordhuis

@addaleax addaleax

@refack refack

@thefourtheye thefourtheye

@cjihrig cjihrig

@joyeecheung joyeecheung

@hashseed hashseed

Assignees
No one assigned
Labels
C++ V8 Engine asm.js backport-requested-v10.x
Projects
None yet
Milestone
No milestone
10 participants
@devsnek @nodejs-github-bot @targos @refack @codebytere @bnoordhuis @thefourtheye @addaleax @cjihrig @joyeecheung
@devsnek
Copy link
Member

commented Nov 14, 2018 •
edited

Currently the only place V8 does this is asm.js compilation:

function AsmModule() {
  'use asm';

  function add(a, b) {
    a = a | 0;
    b = b | 0;

    // should be `return (a + b) | 0;`
    return a + b; // (node:18940) V8: test.js:9 Invalid asm.js: Invalid return type
  }

  return { add: add };
}

In chromium:

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

@nodejs-github-bot nodejs-github-bot added the C++ label Nov 14, 2018

@devsnek devsnek added the V8 Engine label Nov 14, 2018

@devsnek devsnek requested review from addaleax and hashseed Nov 14, 2018

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 14, 2018

Is it testable? Maybe with a message test?
@addaleax

addaleax reviewed Nov 14, 2018
View changes

Copy link
Member

left a comment

Would you mind adding the test case from the PR description as a test here?
Show resolved Hide resolved src/node.cc Outdated

@devsnek devsnek force-pushed the devsnek:feature/v8-warnings branch from 2375408 to 94e10db Nov 14, 2018

@devsnek

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 14, 2018

@targos @addaleax tests added 👍
@addaleax

addaleax approved these changes Nov 14, 2018
View changes

@devsnek

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 14, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/18610/
@refack

refack reviewed Nov 14, 2018
View changes

Copy link
Member

left a comment

👍
Show resolved Hide resolved src/node.cc
Show resolved Hide resolved src/node.cc

@refack refack added the wasm label Nov 14, 2018

@devsnek devsnek removed the wasm label Nov 14, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 14, 2018

refack added the wasm label

I know it's not wasm but it's the most relevant label.

@devsnek devsnek added the asm.js label Nov 14, 2018

@joyeecheung

joyeecheung approved these changes Nov 14, 2018
View changes

Copy link
Member

left a comment

It would probably be even better if ProcessEmitWarningGeneric is less generic on the C++ side and we can just do the overload in JS...but anyway LGTM

(off-topic: we have a wasm label? AND a asm.js label? and they are both green?)
@thefourtheye

thefourtheye approved these changes Nov 14, 2018
View changes

Show resolved Hide resolved src/node.cc Outdated
@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 14, 2018

a wasm label? AND a asm.js label? and they are both green

The asm.js is brand new. And like their subjects, if you look from a distance they look the same ;)

@devsnek devsnek force-pushed the devsnek:feature/v8-warnings branch from 94e10db to 6a17bd4 Nov 14, 2018

@bnoordhuis

bnoordhuis reviewed Nov 14, 2018
View changes

Show resolved Hide resolved src/node.cc Outdated
Show resolved Hide resolved src/node.cc Outdated
Show resolved Hide resolved src/node.cc
Show resolved Hide resolved src/node.cc

@devsnek devsnek force-pushed the devsnek:feature/v8-warnings branch from 6a17bd4 to fc4ec7a Nov 14, 2018

@refack

refack approved these changes Nov 14, 2018
View changes

@devsnek

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 14, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/18619/
@cjihrig

cjihrig approved these changes Nov 15, 2018
View changes

@devsnek
src: emit warnings from V8 
PR-URL: #24365
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Verified
This commit was signed with a verified signature.
@devsnek devsnek Gus Caplan
GPG key ID: F00BD11880E82F0E Learn about signing commits
Loading status checks…
e1aa730

@devsnek devsnek force-pushed the devsnek:feature/v8-warnings branch from fc4ec7a to e1aa730 Nov 16, 2018

@devsnek devsnek merged commit e1aa730 into nodejs:master Nov 16, 2018

@devsnek devsnek deleted the devsnek:feature/v8-warnings branch Nov 16, 2018

targos added a commit that referenced this pull request Nov 18, 2018

@devsnek @targos
src: emit warnings from V8 
PR-URL: #24365
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
ece2a7a

rvagg added a commit that referenced this pull request Nov 28, 2018

@devsnek @rvagg
src: emit warnings from V8 
PR-URL: #24365
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
5d67eec

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

This was referenced Dec 7, 2018

Closed
Closed
Closed
Closed
Closed
@codebytere

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 12, 2019 •
edited

@devsnek do you think that this can/should be backported to v10.x? I added the label but feel free to remove!

@codebytere codebytere added the backport-requested-v10.x label Jan 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.