doc: formalize auto usage in C++ style guide

[addaleax]

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]

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/979/pipeline

[refack]

Conflicts with google style guide & C++ Core Guidelines

[refack]

Conflicts with google style guide & C++ Core Guidelines

[joyeecheung]

Yes please!

[refack]

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

[jasnell]

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

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

[joyeecheung]

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

[addaleax]

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

[refack]

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.

[addaleax]

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.

[joyeecheung]

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

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:

• ES.11: Use auto to avoid redundant repetition of type names
• P.3: Express intent
  With the two first examples:

  for (const auto& x : v) { /* do something with the value of x */ }

  for (auto& x : v) { /* modify x */ }

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:

• Google Reference Arguments - All parameters passed by lvalue reference must be labeled const.
• F.16: For “in” parameters, pass cheaply-copied types by value and others by reference to const
• F.17: For “in-out” parameters, pass by reference to non-const

[refack]

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]

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

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]

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]

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]

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]

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]

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]

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]

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]

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

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

[joyeecheung]

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]

@refack Split out into #23155.

[refack]

Split out into #23155.

Thank you

[addaleax]

Landed in aba7677