-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: emit warnings from V8 #24365
Conversation
Is it testable? Maybe with a message test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding the test case from the PR description as a test here?
2375408
to
94e10db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
The |
94e10db
to
6a17bd4
Compare
6a17bd4
to
fc4ec7a
Compare
PR-URL: nodejs#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>
fc4ec7a
to
e1aa730
Compare
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>
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>
@devsnek do you think that this can/should be backported to |
Currently the only place V8 does this is asm.js compilation:
In chromium:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes