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.

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

doc: formalize auto usage in C++ style guide #23028

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 22, 2018

Adjust heading levels to align with the table of contents, and write down 2 more existing rules (avoiding non-const references and auto under most circumstances).

We generally avoid using auto if not necessary. This formalizes this rules by writing them down in the C++ style guide.

The other part of the PR has been split out into #23155.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 22, 2018
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Conflicts with google style guide & C++ Core Guidelines

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Conflicts with google style guide & C++ Core Guidelines

## Do not include `*.h` if `*-inl.h` has already been included
### Using `auto`

Being explicit about types is preferred over using `auto`.
Copy link
Member

Choose a reason for hiding this comment

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

Yes please!

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the main benefits of adopting an established style guide is to reduce bikeshedding. I recommend the following talk by Herb Sutter (head of the c++ ISO group) https://www.youtube.com/watch?v=hEx5DNLWGgA

Copy link
Member

Choose a reason for hiding this comment

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

I've also found auto convenient and helpful when the type is obvious from context... e.g.

auto traced_value = tracing::TracedValue::Create();

Copy link
Member

@joyeecheung joyeecheung Sep 23, 2018

Choose a reason for hiding this comment

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

@refack Just realized I watched the same argument made by Herb Sutter in 2014 back in 2015 but still my experience in real-world code bases taught me otherwise...not that I am completely against auto, though, but I still prefer explicit types when it helps with readability, especially in code reviews where the diff may be out of context and there is no IDE to help analyzing the types.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell Even in that example one would have to look up what Create() actually returns… for example, it could definitely be either a smart pointer or a regular pointer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point is to reduce "clutter" and allow the reader to focus on the intent of the code.
In most cases the exact type of (for example) traced_value is probably not relevant for review purposes, it might even be a distraction from what the code actually does. From the idea that the more we have to read, we have less focus on the actual point of the change.

After all that is what 100% of code in JS looks like and we manage excellently.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the point is to reduce "clutter" and allow the reader to focus on the intent of the code.

There are other measures, though – in particular, correctness is an important one. By requesting more explicit types, we put that before focusing on intent (as in this example).

After all that is what 100% of code in JS looks like and we manage excellently.

If that were completely true, things like TypeScript wouldn’t exist – which is something that’s used by 46 % of npm users, after all, and the added correctness guarantees are definitely a major reason for that.

Copy link
Member

Choose a reason for hiding this comment

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

@refack

After all that is what 100% of code in JS looks like and we manage excellently.

Not necessarily, for properties and a lot of local variables we initialize them upfront so in the declaration it's already very clear what their types are supposed to be. Personally I would want to see more JSDoc-like annotations in lib (not necessary for simple helpers, but would be very helpful for functions with a lot of arguments named with something like obj, value, handle, .etc).

@refack
Copy link
Contributor

refack commented Sep 22, 2018

I was just about to suggest the opposite, on both clauses. They are both counter-idiomatic with "modern" C++ (c++11 and higher).
From the C++CG:

From the Google style guide:

  • auto - Use auto to avoid type names that are noisy, obvious, or unimportant - cases where the type doesn't aid in clarity for the reader. Continue to use manifest type declarations when it helps readability.
    for (const auto& item : some_map) {
      const KeyType& key = item.first;
      const ValType& value = item.second;
      // The rest of the loop can now just refer to key and value,
      // a reader can see the types in question, and we've avoided
      // the too-common case of extra copies in this iteration.
    }

Both guides have specific rules for function arguments:

@refack
Copy link
Contributor

refack commented Sep 22, 2018

tl;dr IMHO we should improve our style and guidelines by learning from experienced source in order to improve our code correctness, readability, maintainability, and reduce the entry barrier.

@joyeecheung
Copy link
Member

joyeecheung commented Sep 22, 2018

@refack The sections about auto in C++ Core Guidelines and the Google style are not in direct conflict with this patch. In the Google style guide, it does suggest that:

Continue to use manifest type declarations when it helps readability.

And the decision is in favor of explicit types:

auto is permitted when it increases readability, particularly as described below. Never initialize an auto-typed variable with a braced initializer list.

In the C++ Core Guidelines

Exception: Avoid auto for initializer lists and in cases where you know exactly which type you want and where an initializer might require conversion.

They both bring up templates as examples where using auto helps with readability and that's also what this patch suggests.

@addaleax
Copy link
Member Author

I can’t find anything on local (non-parameter) references in the Google style guide, but the policy you linked to is in agreement with this PR for function parameters. I’ve also only very rarely at best seen non-const references in the V8 or Chromium sources.

@refack
Copy link
Contributor

refack commented Sep 22, 2018

They both bring up templates as examples where using auto helps with readability and that's also what this patch suggests.

But the current suggestion is Being explicit about types is preferred over using `auto`.
I suggest a line with an opposite motive (Well I suggest adopting the established guides but) where the gist is: Using `auto` is preferred to being unnecessarily explicit with the exceptions given in both places.

@refack
Copy link
Contributor

refack commented Sep 22, 2018

I'll give an example that came up during review:

for (std::string::size_type i = 0; i < name.size(); ++i) {
  if (name[i] == '_') 
    name[i] = '-';
}

or

for (auto& i : name) {
  if (i == '_') 
    i = '-';
}

Which would be easier for a newcomer to understand? Which one give the compiler more chances to validate the code, and optimize the binary?
I guess this is such a common pattern that it has a corresponding guideline - ES.55: Avoid the need for range checking

@refack
Copy link
Contributor

refack commented Sep 23, 2018

Total serendipity, I stumbled upon this talk about what compilers actually do for these loops, and what they are optimizing
https://www.youtube.com/watch?v=bSkpMdDe4g4

@addaleax
Copy link
Member Author

I suggest a line with an opposite motive (Well I suggest adopting the established guides but) where the gist is: Using `auto` is preferred to being unnecessarily explicit with the exceptions given in both places.

I’m not sure about the others around here, but from the people who review a lot of C++ code on a regular basis, both @joyeecheung and me seem to think that this would impact reviewability and readability negatively.

Which would be easier for a newcomer to understand?

Honestly, I think it depends, both on how “new” the newcomer is and what exactly “understand” means here. The for-range + auto version has the advantage of being easy to write, but it does come with a bit less clarity on what i actually is and that name is the variable that is ultimately being modified here.

Which one give the compiler more chances to validate the code, and optimize the binary?

Both should result in similar code – but we shouldn’t even bother worrying about this question in code that is not performance-critical.

@refack
Copy link
Contributor

refack commented Sep 24, 2018

I’m not sure about the others around here, but from the people who review a lot of C++ code on a regular basis, both @joyeecheung and me seem to think that this would impact reviewability and readability negatively.

I'm not sure what you meant by that, it reads to me like you want that group to stay small and exclusive?

BTW my formal education and experience is in "classic" C++, and all this "modern" stuff is completely new, and still groking our current C++ code IMHO is too hard.

IMHO if our coding guidelines were more in line with established guides (i.e. what you can learn in books and courses, as opposed to word of mouth kept by a small group), the group of C++ writers and reviewers could more more inclusive.

@gireeshpunathil
Copy link
Member

Is this (C++ style guide document) the exact set or subset or a superset of what compiler / make format-cpp already captures? I believe it is none?

@addaleax
Copy link
Member Author

addaleax commented Sep 24, 2018

I’m not sure about the others around here, but from the people who review a lot of C++ code on a regular basis, both @joyeecheung and me seem to think that this would impact reviewability and readability negatively.

I'm not sure what you meant by that, it reads to me like you want that group to stay small and exclusive?

I don’t know how you came to that conclusion based on what I said.

If anything, it’s the opposite – I’d love for the number of people who work on and with our C++ code base to increase. That would make both my life easier and improve Node’s quality on its own.

I’m not sure how to rephrase the above sentence, though. What I meant is that from the existing frequent reviewers, two of us have, based on our experience, come to the conclusion that your suggestion would likely not be an improvement if readability and reviewability are the values by which we judge it. I’d be happy for other people to share their experience as well.

BTW my formal education and experience is in "classic" C++, and all this "modern" stuff is completely new, and still groking our current C++ code IMHO is too hard.

Just to clarify how this relates to the current argument – are you saying these rules would make understanding existing code easier for you?

I understand that our code base is not the most easily accessible one, but as far as I can tell, the main issues that people have are around concepts that are specific to V8 or libuv.

IMHO if our coding guidelines were more in line with established guides (i.e. what you can learn in books and courses, as opposed to word of mouth kept by a small group), the group of C++ writers and reviewers could more more inclusive.

Our guidelines are quite close to the Google ones that you also refer to, especially with regards to the particular items that we’re discussing here.

Is this (C++ style guide document) the exact set or subset or a superset of what compiler / make format-cpp already captures? I believe it is none?

Yeah, it is neither – for example, the rules here can’t be enforced through formatting alone.

@jasnell
Copy link
Member

jasnell commented Sep 24, 2018

@addaleax ... I'm happy with everything in this except the bits about auto. Do you think it would be possible to separate that one bit out to a separate PR that we can discuss separately without blocking the rest of this?

On the auto part, I think we should likely be more explicit about the cases where it's ok. With some examples included.

@addaleax
Copy link
Member Author

@jasnell It looks like this PR is going to be blocked as a whole anyway… I’ll split out the first commit, though, so that we can split the auto section more easily later if it’s necessary.

I’ve also updated the text on auto to be closer to Google’s guidelines + added their example, and I’ll think about more examples that would be good to include here.

addaleax added a commit to addaleax/node that referenced this pull request Sep 24, 2018
Adjust heading levels to align with the table of contents.

Refs: nodejs#23028
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Sep 24, 2018
Adjust heading levels to align with the table of contents.

Refs: nodejs#23028
PR-URL: nodejs#23061
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this pull request Sep 24, 2018
Adjust heading levels to align with the table of contents.

Refs: #23028
PR-URL: #23061
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@joyeecheung
Copy link
Member

Is this (C++ style guide document) the exact set or subset or a superset of what compiler / make format-cpp already captures? I believe it is none?

I believe you are correct, make format-cpp only handles style nits, but it knows nothing about types - it's clang-format underneath, to do proper linting clang-tidy is a better tool although it may be way too slow for our codebase.

@addaleax addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 29, 2018
@addaleax
Copy link
Member Author

@refack Split out into #23155.

@refack
Copy link
Contributor

refack commented Sep 29, 2018

Split out into #23155.

Thank you

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2018
@addaleax
Copy link
Member Author

Landed in aba7677

@addaleax addaleax closed this Sep 30, 2018
@addaleax addaleax deleted the cpp-style-update branch September 30, 2018 15:32
addaleax added a commit that referenced this pull request Sep 30, 2018
We generally avoid using `auto` if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

PR-URL: #23028
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 1, 2018
We generally avoid using `auto` if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

PR-URL: #23028
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
We generally avoid using `auto` if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

PR-URL: #23028
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax added a commit that referenced this pull request Oct 21, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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++. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.