Major updates to Google C++ style guide coming up

[Victor Costan]

Hi folks,

https://github.com/google/styleguide/pull/553 will be exporting a bunch of major updates to the Google C++ style guide, which forms the basis of our own. The major changes are listed in the PR description.

The biggest  change (IMO) is dropping the ban on mutable references. Internally, this was published together with https://abseil.io/tips/177 and http://go/totw/178 (not yet published to abseil.io, sorry). https://abseil.io/tips/176 is also quite relevant.

The PR will be merged soon, and Chromium reviewers will eventually notice and start enforcing the new rules. I think this is a great time to figure out if we want to issue any new Chromium-specific guidance, and do it.

Is it fair to assume we still want to minimize divergence from the Google C++ style guide? In other words, do we want to err on the side of adopting all the changes, unless we have strong reasons to do otherwise?

Is anyone strongly opposed to any of the upcoming changes? If so, would it be ok to spin up a discussion thread per controversial change?

Thank you,

    Victor

[Peter Kasting]

Minimizing divergence is still a good goal. I support the reference change, which should also allow us to resolve the largest remaining Blink style divergence. I don't know what other changes exist (didn't look, on phone); my biggest concern is guidance that might be premature for us due to our lack of C++17.

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP_mGKq7Pf92bjGCBhZoTRR21HZaByrnmxZcwhHqx6i5MMDJxA%40mail.gmail.com.

[Daniel Cheng]

Probably the other two changes that might prompt some discussion are:

- allowing the use of designated initializers in C++20-compliant form

- only allowing kConstantStyle naming for enumerators

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAK8ctizb7EipBM0H3WNyfO13bgWxJcHL4YPeES-PTKhA%40mail.gmail.com.

[Jan Wilken Dörrie]

Daniel did a great job of summarizing the delta in the PR description, and as far as I can see there are no changes that require being on C++17. Most of the C++17 related updates are already on the site, which was done in https://github.com/google/styleguide/pull/471.

Personally I'm excited about the new changes, and hope we can adopt all of them. Allowing mutable references is likely the biggest and most controversial change, but I think it's a good one. The internal go/mutable-reference-parameters gives a bit more background and rationale why the ban was dropped. 

The second biggest one is allowing C++20's designated initializers. This is also a bit controversial, and came up a few times in the past [Designated Initializer Lists, Designated initializers?, crbug/956581]. Previous discussions were always able to refer to https://google.github.io/styleguide/cppguide.html#Nonstandard_Extensions, which is actively discouraging them. However, considering that

* designated initializers are coming in C++20,

* are now blessed by the style guide,

* increase readability and safety (see e.g. https://abseil.io/tips/172 and https://abseil.io/tips/173),

* are supported in clang and gcc since ~forever (https://godbolt.org/z/NcoL3q),

* and we have existing usages in Chromium

I would like to see us allow them for good.

I don't think the new guidance around kConstantStyle is a major change, as the current style guide already states "New code should prefer constant-style naming if possible.".

Best,

Jan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKo26bK9EeHNn5n72vXhx97vOeoZjskm6r%3DozA6_7%3DGqxg%40mail.gmail.com.

[Jeremy Roman]

I wouldn't necessarily have made the same choices myself, but I don't see any strong Chromium-specific reason to diverge, which is the usual bar I would use, so I think we should adopt those when they land in the public version.

It appears that trunk clang enforces the ordering and non-mixing requirements that C++20 has, even in C++14 mode. If that means that the code we write today is likely to be forward-compatible to C++20, then I can live with it, I think. (Though it does increase my eagerness to jump forward past C++14 as soon as reasonable.)

I am pleased that it resolves the Blink style divergence, though amusingly in the opposite direction I had expected. :)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALB5StYigV_G75cqmr68_Wy5_cqRAt44Gu7CReHi%2BhDGb-vxsg%40mail.gmail.com.

[dan...@chromium.org]

I don't think we should try to prevent mutable references, I can't argue against the points made in the decision to use them. I am glad that we can resolve the difference between chromium and blink too!

When (re)writing code however, I would hope that rather than blindly switching from using pointers to references for out-parameters, we reconsider if we could return the result instead. Many of our out-params come from legacy code at this point.

I don't think we have anything to change in regard to kConstantStyle. It doesn't seem worth rewriting all our old enums.

On Wed, May 20, 2020 at 10:09 AM Jeremy Roman <jbr...@chromium.org> wrote:

I wouldn't necessarily have made the same choices myself, but I don't see any strong Chromium-specific reason to diverge, which is the usual bar I would use, so I think we should adopt those when they land in the public version.

It appears that trunk clang enforces the ordering and non-mixing requirements that C++20 has, even in C++14 mode. If that means that the code we write today is likely to be forward-compatible to C++20, then I can live with it, I think. (Though it does increase my eagerness to jump forward past C++14 as soon as reasonable.)

Thanks for this point. I would like confirmation of this as well.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13fQXcpT1rhxUxh4M2-8zb0cE%2BJeQa1ej3JBDuO_rOeXiw%40mail.gmail.com.

[Nico Weber]

The non-const reference and enum changes happen to reduce style diff between chrome and blink, which is great.

We do have an open bug somewhere already to eventually rewrite all all-caps enums to kFoo enums. I agree it's not high priority. We can just keep that bug open.

Overall, I'm happy that this makes chrome, blink, and google styles more similar. I don't think we should add any exceptions to the upstream changes.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaT6at69SzSh0etCLz0XOGcaKMxihQkYWCwpJA9iz%2BDysQ%40mail.gmail.com.

[Mark Mentovai]

I think that we should adopt substantially all of the upstream changes without making exceptions.

The one thing I’d like to call out is that the style guide is becoming permissive of (void) casts. I believe that ignore_result is clearer and would prefer to stick with it. (void) and ignore_result are only intended to circumvent WARN_UNUSED_RESULT, which is not a standard feature of the language, and is itself implemented with a compiler-specific __attribute__. I would argue that (void) used for this purpose is not really a feature of the language either, idiomatic as it may be, and we’d be better served by retaining use of ignore_result with its added documentary value and greater discoverability in the event that the language does grow standard support for something like WARN_UNUSED_RESULT and we find ourselves wanting to undertake a mass conversion.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiEDpKp%2BZ-LdcfLJmk_BQ-%3D1AGY7zf9aM2yNzo2o6B4iAQ%40mail.gmail.com.

[Roland McGrath]

Note that C++17 [[nodiscard]] is the equivalent of [[gnu::warn_unused_result]] and so whenever C++17 is adopted, it will be a standard part of the language in use.  The cast to void is explicitly the only way to suppress the warning for the standard feature.  Obviously a named wrapper for cast to void is just fine too and that's better implicit documentation of the intent.  But cast to void is certainly idiomatic standard use for [[nodiscard]].

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHk1GDwhD0XsSVB%3D%3Dus8wL-sAZpKpDMLURPFeLPTaW5bDhc5CA%40mail.gmail.com.

[Mark Mentovai]

Thanks, Roland, I missed that addition.

[K. Moon]

Daniel's PR was approved, so the guide updates are live now.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHk1GDxFBAKKvHKkX0hDJyHnfbhvTaweUQcg%3D3tu9bBiy8F2kw%40mail.gmail.com.

[Peter Kasting]

On Tue, May 26, 2020 at 12:29 PM K. Moon <km...@chromium.org> wrote:

Daniel's PR was approved, so the guide updates are live now.

Woah, the TOC at the top went away.  That sucks, I used that a lot :/

It seems like at the minimum we should probably clarify in our style guide that for the time being, Chromium targets C++14, not 17, and for further detail in the future people can always look at chromium-cpp.appspot.com.  Then we should send out a mail noting the changes, particularly the references change, and follow up with the Blink style owners about formally resolving/removing the its in the Blink style guide relating to this.  Am I missing any necessary steps?  If not I can probably own at least some of this process.

PK

[Daniel Cheng]

Doh. I'll submit a PR to add the TOC back.

(The script that derives the public version of the guide doesn't know about the TOC block, and I regenerated the CL once to make a few small fixes but forgot to restore the TOC.)

Daniel

[Peter Kasting]

On Tue, May 26, 2020 at 3:30 PM Daniel Cheng <dch...@chromium.org> wrote:

Doh. I'll submit a PR to add the TOC back.

(The script that derives the public version of the guide doesn't know about the TOC block, and I regenerated the CL once to make a few small fixes but forgot to restore the TOC.)

Oh, thank you, I thought it was an intentional change :).

PK 

[Victor Costan]

On Tue, May 26, 2020 at 2:22 PM Peter Kasting <pkas...@google.com> wrote:

The TOC is back :)

The steps above seem reasonable to me. Copy-pasting them below with numbers, for easy reference.

1. Clarify in our style guide that for the time being, Chromium targets C++14, not 17, and for further detail in the future people can always look at chromium-cpp.appspot.com. 

2. Send out a mail noting the changes, particularly the references change.

3. Follow up with the Blink style owners about formally resolving/removing the bits in the Blink style guide relating to this.

PK, which steps would you be willing to own? I can try to take care of the rest.

Thank you,

    Victor

[Peter Kasting]

I'll try and write a CL for (1) today.  Once that lands I can do (2).

Perhaps you can take (3) starting immediately?  Depending on what updates the Blink folks want to make, we might want to try to wrangle all those before sending mail (i.e. swap (2) and (3) in the order).

PK 

[Mark Mentovai]

Peter Kasting wrote:

On Thu, May 28, 2020 at 4:21 AM Victor Costan <pwn...@chromium.org> wrote:

On Tue, May 26, 2020 at 2:22 PM Peter Kasting <pkas...@google.com> wrote:

On Tue, May 26, 2020 at 12:29 PM K. Moon <km...@chromium.org> wrote:

Daniel's PR was approved, so the guide updates are live now.

Woah, the TOC at the top went away.  That sucks, I used that a lot :/

It seems like at the minimum we should probably clarify in our style guide that for the time being, Chromium targets C++14, not 17, and for further detail in the future people can always look at chromium-cpp.appspot.com.  Then we should send out a mail noting the changes, particularly the references change, and follow up with the Blink style owners about formally resolving/removing the its in the Blink style guide relating to this.  Am I missing any necessary steps?  If not I can probably own at least some of this process.

The TOC is back :)

The steps above seem reasonable to me. Copy-pasting them below with numbers, for easy reference.

1. Clarify in our style guide that for the time being, Chromium targets C++14, not 17, and for further detail in the future people can always look at chromium-cpp.appspot.com. 

2. Send out a mail noting the changes, particularly the references change.

3. Follow up with the Blink style owners about formally resolving/removing the bits in the Blink style guide relating to this.

PK, which steps would you be willing to own? I can try to take care of the rest.

I'll try and write a CL for (1) today.  Once that lands I can do (2).

Thanks, Peter.

If there are reasons (such as toolchain support) that we can’t yet move to C++17, can you include them in your (1)? Ideally, this would be tracked by a bug that you could link to.

If there are no reasons to hold back on C++17 other than “we haven’t had the discussion yet,” then we should start the discussion.

[Peter Kasting]

On Thu, May 28, 2020 at 7:56 AM Mark Mentovai <ma...@chromium.org> wrote:

If there are reasons (such as toolchain support) that we can’t yet move to C++17, can you include them in your (1)? Ideally, this would be tracked by a bug that you could link to.

If there are no reasons to hold back on C++17 other than “we haven’t had the discussion yet,” then we should start the discussion.

This is the "NaCl toolchain doesn't support C++17, likely stuck for at least another year" issue; you're right, I should summarize/cite the relevant bits in the style guide change and the mail.  I'll try and cross-link things better here.

PK

[dan...@chromium.org]

Thank sounds great. Thanks Peter and Victor and Daniel!

 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAjDADbi-NjdJhOSUDpgyB%3DXLUs3Pn%2BsUHJCG8U37gvhA%40mail.gmail.com.

[Victor Costan]

I think all the steps outlined here have been carried out, and this thread is wrapped up.

Thank you PK for doing all the hard work!

For historical reference, the Chromium-wide announcement is at https://groups.google.com/a/chromium.org/d/topic/chromium-dev/me0oHd69td8/discussion

    Victor

[Michael Lippautz]

[Sorry for cross-posting but didn't get a reply on the blink-dev@ thread.]

As for mutable references: Will the rule be adjusted globally (cpplint.py) or are we supposed to adjust dependent projects manually by adding "runtime/references" to a linter allow list?

[dan...@chromium.org]

On Thu, Jun 18, 2020 at 2:09 PM Michael Lippautz <mlip...@chromium.org> wrote:

[Sorry for cross-posting but didn't get a reply on the blink-dev@ thread.]

As for mutable references: Will the rule be adjusted globally (cpplint.py) or are we supposed to adjust dependent projects manually by adding "runtime/references" to a linter allow list?

I believe we need to remove runtime/references from depot_tools: https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?rcl=13d717d087e227dee2f928f2368c0120688b2ac9&l=225

Thanks for bringing this up. Would you be able to test if that indeed allows them in our PRESUBMIT linted code?

[dan...@chromium.org]

On Thu, Jun 18, 2020 at 2:14 PM <dan...@chromium.org> wrote:

On Thu, Jun 18, 2020 at 2:09 PM Michael Lippautz <mlip...@chromium.org> wrote:

[Sorry for cross-posting but didn't get a reply on the blink-dev@ thread.]

As for mutable references: Will the rule be adjusted globally (cpplint.py) or are we supposed to adjust dependent projects manually by adding "runtime/references" to a linter allow list?

I believe we need to remove runtime/references from depot_tools: https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?rcl=13d717d087e227dee2f928f2368c0120688b2ac9&l=225

OOPS that is the upstream script. What I should have said is we should -runtime/references over here: https://cs.chromium.org/chromium/tools/depot_tools/presubmit_canned_checks.py?rcl=13d717d087e227dee2f928f2368c0120688b2ac9&l=21

And any callers to CheckChangeLintsClean() that pass a filter should also include it.

[Peter Kasting]

On Thu, Jun 18, 2020 at 11:14 AM <dan...@chromium.org> wrote:

On Thu, Jun 18, 2020 at 2:09 PM Michael Lippautz <mlip...@chromium.org> wrote:

[Sorry for cross-posting but didn't get a reply on the blink-dev@ thread.]

As for mutable references: Will the rule be adjusted globally (cpplint.py) or are we supposed to adjust dependent projects manually by adding "runtime/references" to a linter allow list?

I believe we need to remove runtime/references from depot_tools: https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?rcl=13d717d087e227dee2f928f2368c0120688b2ac9&l=225

I had intentionally not yet replied to the earlier thread since I've had an open CL to do precisely this for a few days, and I was hoping to land it and then say something like "thanks, done": https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2248714

However, since that CL has to date been radio silence, I'll mention it here so no one else goes and writes a duplicate.  In summary, though, yes, we should fix this globally.

PK 

[Peter Kasting]

On Thu, Jun 18, 2020 at 11:38 AM Peter Kasting <pkas...@google.com> wrote:

On Thu, Jun 18, 2020 at 11:14 AM <dan...@chromium.org> wrote:

On Thu, Jun 18, 2020 at 2:09 PM Michael Lippautz <mlip...@chromium.org> wrote:

As for mutable references: Will the rule be adjusted globally (cpplint.py) or are we supposed to adjust dependent projects manually by adding "runtime/references" to a linter allow list?

I believe we need to remove runtime/references from depot_tools: https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?rcl=13d717d087e227dee2f928f2368c0120688b2ac9&l=225

I had intentionally not yet replied to the earlier thread since I've had an open CL to do precisely this for a few days, and I was hoping to land it and then say something like "thanks, done": https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2248714

...And this is now landed.  If you see further presubmit complaints about this, please let me know.

PK

[Michael Lippautz]

Thanks for taking care of this!

To update this thread: I was specifically interested in how V8 would adopt the rule as it's also using Google/Chromium style guide. Globally adjusting cppylint.py would've worked great. Since this will not happen directly now, I am adjusting V8's presubmit checks.

[K. Moon]

One fairly subtle change in this update that I just noticed recently:

Under "Declaration Order,"

"..., constructors, assignment operators, destructor, ..."

became

"..., constructors and assignment operators, destructor, ..."

https://github.com/google/styleguide/commit/7a7a2f510efe7d7fc5ea8fbed549ddb31fac8f3e#diff-26120df7bca3279afbf749017c778545L1685

So if you like doing:

A(const A&);
A& operator=(const A&);
A(A&&);
A& operator=(A&&);

instead of:

A(const A&);
A(A&&);
A& operator=(const A&);
A& operator=(A&&);

the style guide no longer thinks you're wrong. :-)