Skip to content

src: replace autos in node_contextify.cc#38644

Closed
XadillaX wants to merge 3 commits intonodejs:masterfrom
XadillaX:node_contextify_auto
Closed

src: replace autos in node_contextify.cc#38644
XadillaX wants to merge 3 commits intonodejs:masterfrom
XadillaX:node_contextify_auto

Conversation

@XadillaX
Copy link
Contributor

@XadillaX XadillaX commented May 12, 2021

No description provided.

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels May 12, 2021
@legendecas
Copy link
Member

legendecas commented May 12, 2021 •
edited
Loading

These occurrences of auto are either straightforward to infer the variable types in the same statement or reduce the burden to expand the lambda type. I don't find the necessity or improvements introduced by this change, can you elaborate on the intention of the change?

@nodejs-github-bot

This comment has been minimized.

Sign in to view
addaleax
addaleax requested changes May 12, 2021
View reviewed changes
Copy link
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.

I’m generally a fan of avoiding auto, but @legendecas is right that these are straightforward to infer (which is basically the only case in which I’m okay with auto :))

That being said, this has already two approvals and we should not just use std::function<> for lambda types if we don’t have to (it creates extra objects and extra code, and, potentially, adds heap allocations), so I’ll mark this as request changes just to avoid that. I don’t have a strong opinion on the other cases.

@addaleax
Copy link
Member

addaleax commented May 12, 2021

Btw, this is why we have: https://github.com/nodejs/node/blob/master/doc/guides/cpp-style-guide.md#using-auto

@XadillaX XadillaX requested a review from addaleax May 13, 2021 02:41
@XadillaX
Copy link
Contributor Author

XadillaX commented May 13, 2021

I’m generally a fan of avoiding auto, but @legendecas is right that these are straightforward to infer (which is basically the only case in which I’m okay with auto :))

That being said, this has already two approvals and we should not just use std::function<> for lambda types if we don’t have to (it creates extra objects and extra code, and, potentially, adds heap allocations), so I’ll mark this as request changes just to avoid that. I don’t have a strong opinion on the other cases.

done

addaleax
addaleax approved these changes May 13, 2021
View reviewed changes
Copy link
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.

🤷‍♀️

@mhdawson

This comment has been minimized.

Sign in to view
1 similar comment
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels May 14, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 19, 2021

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

@mhdawson
Copy link
Member

mhdawson commented May 19, 2021

Looks like only failure is known issue being discussed in: #38226, will land.

mhdawson
mhdawson approved these changes May 19, 2021
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request May 19, 2021
@XadillaX @mhdawson
src: replace autos in node_contextify.cc
a742c40 
PR-URL: #38644
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented May 19, 2021

Landed in a742c40

@mhdawson mhdawson closed this May 19, 2021
danielleadams pushed a commit that referenced this pull request May 31, 2021
@XadillaX @danielleadams
src: replace autos in node_contextify.cc
a47fd67 
PR-URL: #38644
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@danielleadams danielleadams mentioned this pull request May 31, 2021
Merged
richardlau pushed a commit that referenced this pull request Jul 16, 2021
@XadillaX @richardlau
src: replace autos in node_contextify.cc
e61feb8 
PR-URL: #38644
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 19, 2021
@XadillaX @richardlau
src: replace autos in node_contextify.cc
c33e381 
PR-URL: #38644
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
@XadillaX @richardlau
src: replace autos in node_contextify.cc
7d82200 
PR-URL: #38644
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@richardlau richardlau mentioned this pull request Jul 20, 2021
Merged
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@XadillaX @foxxyz
src: replace autos in node_contextify.cc
71a2767 
PR-URL: nodejs#38644
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@mhdawson mhdawson mhdawson approved these changes

@juanarbol juanarbol juanarbol approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@XadillaX @legendecas @nodejs-github-bot @addaleax @mhdawson @jasnell @juanarbol @RaisinTen