Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
src: replace autos in node_api.cc#38852
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
bl-ue left a comment
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.
All looks good, but why not use auto, exactly?
XadillaX commented May 30, 2021
https://github.com/nodejs/node/blob/master/doc/guides/cpp-style-guide.md#using-auto |
bl-ue commented May 30, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ah, makes perfect sense.
Those are points that I've thought about myself. Of course, when you're using an IDE, it'll tell you what |
| {0, 0}); | ||
| } else{ | ||
| auto node_async_context = | ||
| v8impl::AsyncContext* node_async_context = |
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.
By the way, I think the results of casts and new ... can qualify as “obvious” types. I don’t mind changing these, but I’d avoid doing it everywhere because of the churn it creates.
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 just consider that "Forget auto keywords in C++ except lambda".
nodejs-github-bot commented May 31, 2021
nodejs-github-bot commented May 31, 2021
mhdawson left a comment
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.
LGTM
PR-URL: #38852 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
XadillaX commented Jun 1, 2021
Landed in 46acbb0 |
PR-URL: #38852 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #38852 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #38852 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #38852 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#38852 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
No description provided.