Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
src: handle bad callback in asyc_wrap#31946
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
src/async_wrap-inl.h Outdated
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.
We don’t add using statements to headers (we should probably have a linter rule against that, at least for the v8 symbols…)
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.
@addaleax, grep "using v8::" src/*.h
I am seeing this pattern in few files already. If I remove using phrase, is there any way I can elevate ?
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.
Yes, these other files should also not use it.
There are no shorthands in that case, we do spell out the entire type in headers.
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.
@addaleax,thanks. Reverted the changes. What is the rationale for this ?
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.
I mean, why we want to skip for headers ? Is there any performance reason or something else ?
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.
@HarshithaKP Because then the symbols are available in any source file that includes this header, even if that’s not obvious. For example, this code would fail to compile if example.h has a using v8::Array; line:
#include"example.h"classArray{};Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Align with the MaybeLocal<> API contract
2a91f94 to fc53c8dCompareUh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Mar 4, 2020
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
gireeshpunathil commented Mar 5, 2020
landed in 757e203 |
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
src: handle bad callback in asyc_wrap
Align with the MaybeLocal<> API contract
Refs:
node/deps/v8/include/v8.h
Lines 345 to 349 in 9403250
addresses an @addaleax 's TODO
src: elevate v8 namespaces
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes