FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: replace `auto`s in node_contextify.cc by XadillaX · Pull Request #38644 · nodejs/node · GitHub

/ node Public

src: replace autos in node_contextify.cc - #38644

Closed
XadillaX wants to merge 3 commits into
nodejs:masterfrom
XadillaX:node_contextify_auto
ConversationCommits (3)ChecksFiles changed
Closed

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

Conversation

XadillaX commented May 12, 2021

Copy link
Copy Markdown
Contributor

No description provided.

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 commented May 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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?

This comment has been minimized.

Sign in to view
addaleax requested changes May 12, 2021
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 12, 2021

Copy link
Copy Markdown
Member

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

XadillaX requested a review from addaleax May 13, 2021 02:41

XadillaX commented May 13, 2021

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

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🤷‍♀️

This comment has been minimized.

Sign in to view
1 similar comment

This comment has been minimized.

Sign in to view
addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels May 14, 2021

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

nodejs-github-bot commented May 19, 2021

Copy link
Copy Markdown
Collaborator

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

mhdawson commented May 19, 2021

Copy link
Copy Markdown
Member

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

mhdawson approved these changes May 19, 2021
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson pushed a commit that referenced this pull request May 19, 2021
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 commented May 19, 2021

Copy link
Copy Markdown
Member

Landed in a742c40

mhdawson closed this May 19, 2021
danielleadams pushed a commit that referenced this pull request May 31, 2021
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 mentioned this pull request May 31, 2021
Merged
richardlau pushed a commit that referenced this pull request Jul 16, 2021
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
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
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 mentioned this pull request Jul 20, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
juanarbol juanarbol approved these changes
RaisinTen RaisinTen approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL