Skip to content
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.

Sign up for GitHub

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

Jump to bottom

src: replace autos in node_contextify.cc #38644

Closed
wants to merge 3 commits into from
Closed

src: replace autos in node_contextify.cc #38644

wants to merge 3 commits into from

Conversation

XadillaX
Copy link
Contributor

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

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

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

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

@mhdawson
Copy link
Member

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 <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@mhdawson
Copy link
Member

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 <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@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 <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@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 <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@juanarbol juanarbol juanarbol approved these changes

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell 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