Chromium style guide simplification proposals

[Peter Kasting]

Style arbiters,

As part of Chrome's diversity & inclusion efforts, I'm trying to make onboarding for new team members easier.  One way to help this a bit is to simplify the Chromium style guide; the fewer rules new team members have to remember and follow, the easier it is to ramp up.

Accordingly, I'd like to propose a lot of simplifications.  I could split these into separate threads, but I'm hoping one thread is easier to handle.  Let me know if I'm wrong :)

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

• Test-only free functions should generally live within a test_support target.

• #if defined(UNIT_TEST) is problematic and discouraged.

The first bullet has some value.  The second is a combination of obvious and overly-pedantic.  The third is obscure and unimportant.  The fourth is rarely attempted and probably doesn't need to be stated.

I don't think the value of the first bullet is sufficient to keep this section.

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

• Don't use else after return.

These are all pedantic (save the bit about using the override specifier, which I think our tools enforce).  Their benefit doesn't justify forcing people to learn and abide by them.

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

...or elsewhere; this is more a "how to" than a style rule and doesn't belong in this file.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)
  

The first is implicit in common usage, and if people really violated it, fine.  The second isn't really a style rule.  The third is redundant with the Google style guide.  The fourth is hopefully unnecessary at this point with wstring being rare across the codebase.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

PK

[Devlin]

Thanks for kicking this off, Peter!  I agree with you that a lot of these don't have benefit, and should be removed.  There are a few I think I'd like to keep, listed below.

Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

I think these have very real, tangible benefit for documenting (and enforcing, via presubmit) a function is test-only.  I've caught layering violations in the past with these.  It's also not immutable (you can take something that was TestingOnly and make it general if it warrants it, but it forces the reviewer to look at it harder - IMO, WAI.) 

When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

I like this, for methods of the same class.  In Chromium, we have a bunch of classes that implement multiple interfaces (for better or worse), and it's helpful to see where each of those come from.  Admittedly, this is also (typically, depending on platform) available by codesearch, but that's not always the case, and I think it's a) very low cost b) somewhat good practice to group similar overridden methods together in a single class.

Note: I don't think we need all overridden methods in one section, but think we could rephrase this that each overridden method must be prefixed.  That is, if Bar overrides Foo, Bar should have `// Foo:` comment over overridden function (groups).

Don't use else after return.

I personally think this is always good advice, and makes code generally more readable.

As always, just my $0.02 on all of these.  And I think the majority here you've highlighted are ones that could be either removed, or moved to the less pedantic "do's and don't's" page.  Thank you again for putting this together, and making it easier for people to onboard! 

[Peter Kasting]

On Fri, Nov 9, 2018 at 7:59 PM Devlin <rdevlin...@chromium.org> wrote:

When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

I like this, for methods of the same class.  In Chromium, we have a bunch of classes that implement multiple interfaces (for better or worse), and it's helpful to see where each of those come from.  Admittedly, this is also (typically, depending on platform) available by codesearch, but that's not always the case, and I think it's a) very low cost b) somewhat good practice to group similar overridden methods together in a single class.

Note: I don't think we need all overridden methods in one section, but think we could rephrase this that each overridden method must be prefixed.  That is, if Bar overrides Foo, Bar should have `// Foo:` comment over overridden function (groups).

I don't think Chromium does things differently from google3 here in a way that justifies a style rule.

Also, the current override situation is highly confused.  People use all sorts of labels, and disagree on whether you group and label based on the immediate superclass or the original base ancestor the function came from.  I don't think our current style rule is really helping, and I think it's telling that the specific thing you seem to want is more about labeling and less about "put in one section and use override", which is what the rule actually says.

Don't use else after return.

I personally think this is always good advice, and makes code generally more readable.

I do too, but again, I don't think there's anything about Chromium that justifies diverging from Google style on this.

If we removed this rule, I would still note this in my code reviews, just as "Nit: else unnecessary here" instead of "No else after return (see <link>)".

PK

[Nico Weber]

While I like several of these, I agree that having fewer diffs from the regular Google style guide is good. So I agree with almost all of this.

The wstring one is the one that looks vaguely chrome-specific and that could be justified to keep around. We still have 1.5k files with wstrings, and I've pointed to that rule with in the last year iirc. (But I don't have a strong opinion about it.)

--
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 post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDDR5S85DB26xPO45PpZKEZpJz7C8T%3D-%2B0wv2VDeQFGxA%40mail.gmail.com.

[Jeremy Roman]

I think this is all pretty reasonable.

On Fri, Nov 9, 2018 at 8:04 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Style arbiters,

As part of Chrome's diversity & inclusion efforts, I'm trying to make onboarding for new team members easier.  One way to help this a bit is to simplify the Chromium style guide; the fewer rules new team members have to remember and follow, the easier it is to ramp up.

Accordingly, I'd like to propose a lot of simplifications.  I could split these into separate threads, but I'm hoping one thread is easier to handle.  Let me know if I'm wrong :)

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

• Test-only free functions should generally live within a test_support target.

• #if defined(UNIT_TEST) is problematic and discouraged.

The first bullet has some value.  The second is a combination of obvious and overly-pedantic.  The third is obscure and unimportant.  The fourth is rarely attempted and probably doesn't need to be stated.

I do think it's useful to know that the exact string ForTesting has PRESUBMIT value, as I think that's non-obvious otherwise. I don't mind whether it lives here or elsewhere.

I'm not sure #2 is entirely obvious, though https://abseil.io/tips/135 might be a clearer statement of it than the one we have here.

Aside: I'd kinda like to file a GoodFirstBug to just remove the existing uses of UNIT_TEST as there aren't that many and it seems likely that they can all be replaced with something more usual (like the ForTesting suffix and no preprocessor guard).

 

I don't think the value of the first bullet is sufficient to keep this section.

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

• Don't use else after return.

These are all pedantic (save the bit about using the override specifier, which I think our tools enforce).  Their benefit doesn't justify forcing people to learn and abide by them.

The first is, as you say, partially enforced by the [chromium-style] plugin. I've found it most useful when a class derives multiple pure virtual base classes (e.g., client/delegate interfaces), to mark what came from where. I've seen reviewers ask for this fairly frequently, and having somewhere to point authors at seems more useful than surprising them at review time. But I don't feel very strongly.

I agree that the second might be a little too prescriptive, especially since we seldom do this in the part of the code I work in.

I actually dislike the "Don't use else after return", as I've had reviewers ask for it even in cases where I thought it made the code less readable. IMHO he more useful variant of it is probably "early-return for things that are precondition checks etc. that are not on the main execution path of the function", but that's really fuzzy. In any case I wouldn't mind losing it.

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

...or elsewhere; this is more a "how to" than a style rule and doesn't belong in this file.

Sure.

 

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)
  

The first is implicit in common usage, and if people really violated it, fine.  The second isn't really a style rule.  The third is redundant with the Google style guide.  The fourth is hopefully unnecessary at this point with wstring being rare across the codebase.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

PK

--

[Gabriel Charette]

Sounds good to me, thanks for taking the time to go over this. Just two things below.

On Fri, Nov 9, 2018 at 8:04 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Style arbiters,

As part of Chrome's diversity & inclusion efforts, I'm trying to make onboarding for new team members easier.  One way to help this a bit is to simplify the Chromium style guide; the fewer rules new team members have to remember and follow, the easier it is to ramp up.

Accordingly, I'd like to propose a lot of simplifications.  I could split these into separate threads, but I'm hoping one thread is easier to handle.  Let me know if I'm wrong :)

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

• Test-only free functions should generally live within a test_support target.

• #if defined(UNIT_TEST) is problematic and discouraged.

The first bullet has some value.  The second is a combination of obvious and overly-pedantic.  The third is obscure and unimportant.  The fourth is rarely attempted and probably doesn't need to be stated.

I don't think the value of the first bullet is sufficient to keep this section.

Agree with others that should at least keep ForTesting => presubmits if used outside tests.

 

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

• Don't use else after return.

These are all pedantic (save the bit about using the override specifier, which I think our tools enforce).  Their benefit doesn't justify forcing people to learn and abide by them.

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

...or elsewhere; this is more a "how to" than a style rule and doesn't belong in this file.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)
  

The first is implicit in common usage, and if people really violated it, fine.  The second isn't really a style rule.  The third is redundant with the Google style guide.  The fourth is hopefully unnecessary at this point with wstring being rare across the codebase.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

I care strongly about this rule. Given it's more importantly about readability (ownership passing) more than correctness (avoiding atomic ref bump); I'd vote for keeping it in the style guide.

PS: I guess the Dos and Don'ts page should start with a cross-reference to the style guide? And the style-guide end with a cross-reference to Dos and Don'ts? Then the latter just becomes a continuation of the former?

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

PK

--

[dan...@chromium.org]

Thanks for going through all these things.

On Fri, Nov 9, 2018 at 8:04 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Style arbiters,

As part of Chrome's diversity & inclusion efforts, I'm trying to make onboarding for new team members easier.  One way to help this a bit is to simplify the Chromium style guide; the fewer rules new team members have to remember and follow, the easier it is to ramp up.

Accordingly, I'd like to propose a lot of simplifications.  I could split these into separate threads, but I'm hoping one thread is easier to handle.  Let me know if I'm wrong :)

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

I think this is a very important tool for new folks to learn, and if not from the style guide, from where? I also do think we should keep an expectation that all testing-only methods are marked as such, so it belongs in the style guide.

 

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

This is more of an explanation of "but what about constructors" for the above rule. I don't think it's harmful to keep, though there are other ways to do this too (static ForTesting factory method). So probably could be simplified to like "Test-only constructors can not be given a ForTesting suffix, but should be hidden and exposed only to tests via other means." perhaps. Explaining the ways to do so might be good content for Do's And Don't's?

• Test-only free functions should generally live within a test_support target.

 If the above moved to Do's and Don't's this could too.

• #if defined(UNIT_TEST) is problematic and discouraged.

+1 Jeremy's idea to just remove all of these, and PRESUBMIT they don't occur. In the meantime, I would make this language stronger so it fits the prescriptive intent of the style guide more clearly.

 

The first bullet has some value.  The second is a combination of obvious and overly-pedantic.  The third is obscure and unimportant.  The fourth is rarely attempted and probably doesn't need to be stated.

I would caution against using the fact something isn't done often as a reason to not include it in the style guide. Ideally nothing the style guide forbids happens often, in which case all the rules go away, but it could be the reason why it happens rarely.

 

I don't think the value of the first bullet is sufficient to keep this section.

I do think that how we write code to interface with tests is one of the most important things for a new member to learn and understand, and this seems to be quite specific to chrome (clang plugin, PRESUBMITs etc)

 

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

This is generally done and contributes greatly to code readability. +1 to allowing multiple groups (some public some private for example) but I strongly wish to keep this. 

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

Google style guide used to say "one class per file" and lost that. This would have been an extension to reinforce that rule, but is not making a rule of its own. So I guess it could go away. I am sad about it but whatever.

• Don't use else after return.

I don't know why chrome needs this as its own rule when Google doesn't. Do's and Don't's seems appropriate given we cared enough to put it here?

 

These are all pedantic (save the bit about using the override specifier, which I think our tools enforce).  Their benefit doesn't justify forcing people to learn and abide by them.

How we structure our header files has an enormous impact on readability and the grouping (with a comment) of interface methods is one of the strongest things we do IMO. When a method leaves the group it often ends up adding friction to understanding the structure of the code.

 

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

...or elsewhere; this is more a "how to" than a style rule and doesn't belong in this file.

This is a requirement of our code, and is chrome-specific (yay lots of platforms). I've seen this done wrong, and tooling doesn't always see it, until it becomes a problem at some point. I'd vote to keep it.

 

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

IIRC size_t was not mentioned in the Google style guide before, and this sounds like an evolution of that rule as we needed to say less ourselves. This feels more like a good practice than something presciptive, ie Do's and Don't's if anything. I'm not sure that it's a high value rule as you say. 

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.

Nicely informational, and probably an important thing for new folks to Chrome to think about given all our platforms. I see new folks forget that size_t can be a different size sometimes. So I do think this is a good thing to have somewhere. I agree it's not a style rule though, where should "advice for new people" be, Do's and Don't's?

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

+1. Redundant with Google style guide.

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)
  

This is another case I think of the style guide is why it isn't used, so the rule is useful. Also very important for newcomers to the codebase. From my experience, just because $FOO is rare (or even banned) throughout the code base, it does not imply it will not be used in some area of the code.

The first is implicit in common usage, and if people really violated it, fine.  The second isn't really a style rule.  The third is redundant with the Google style guide.  The fourth is hopefully unnecessary at this point with wstring being rare across the codebase. Rules like this are important to keep things consistent, as a forcing function when it is violated to validate the effort to switch to back into conformance.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

This is an extension of a Google style guide rule, so it seems like it belongs in the style guide to me.

 

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

I believe this is a good rule to keep consistency in the codebase. We decided previously to use this convention and a rule means we expect code to follow it - personally I hope that we do. Also, seems important to call out for new folks who may be used to different conventions.

 

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

+1 to improving docs always. These do come with lots of examples which I do like though.

 

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

+1 the const explanations are awesome and could live in their own doc. We could link to it from there to help keep its visibility?

Thanks,

Dana

[bruce...@google.com]

I understand that off_t, uint32_t, and uint64_t will (necessarily) not always be the same size as size_t, but do we support any platforms where size_t, ptrdiff_t, and intptr_t are different sizes? Different signed/unsigned yes, but different sizes?

A more common error that I see is the assumption that sizeof(long) == sizeof(void*). This is true on all platforms except 64-bit Windows and is a somewhat common source of errors there. In particular:

size_t x = 1L << shift_amount;

is broken. 1LL works on all known platforms. static_cast<size_t>(1) is more precise but is painfully verbose.

[Karl Wiberg]

On Thu, Nov 15, 2018 at 12:39 AM brucedawson via cxx <c...@chromium.org> wrote:

I understand that off_t, uint32_t, and uint64_t will (necessarily) not always be the same size as size_t, but do we support any platforms where size_t, ptrdiff_t, and intptr_t are different sizes? Different signed/unsigned yes, but different sizes?

A more common error that I see is the assumption that sizeof(long) == sizeof(void*). This is true on all platforms except 64-bit Windows and is a somewhat common source of errors there. In particular:

size_t x = 1L << shift_amount;

is broken. 1LL works on all known platforms. static_cast<size_t>(1) is more precise but is painfully verbose.

The Google style guide suggests

  size_t x = size_t{1} << shift_amount;

https://google.github.io/styleguide/cppguide.html#Casting

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/79a67719-dc00-4b65-a6c3-d9d0780c8d0e%40chromium.org.

--

Karl Wiberg

Software Engineer

kwi...@google.com

+46 70 696 1024

[Bruce Dawson]

Thanks for pointing that out. size_t{1} is much better - precise without being extravagantly huge.

--

Bruce Dawson

[Peter Kasting]

Trying to summarize current feedback and give a plan to move forward.  Please respond if you object to my summary, have more to say, or want to give feedback on "pending feedback" items (silence == assent).

NOTE: I added one more suggested bullet to remove in the "types" section below.

On Fri, Nov 9, 2018 at 5:04 PM Peter Kasting <pkas...@google.com> wrote:

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

For this bullet, everyone agrees on the utility of this functionality, and that it's nice to mention to people (somewhere) that a presubmit will check it.

Plan: Keep this bullet in the style guide. 

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

As Dana noted, these aren't the only route for exposing "test-only constructors".  Her suggestion was to leave some comment in here about how constructor functionality should be exposed some other way.  It seems like that will be obvious if people run into this in the wild.  I'm inclined to just drop this, although if people truly believe it has value, my suggestion would be to simply say "Test-specific constructors cannot be suffixed with ForTesting; use a ForTesting factory method instead."  Opinions?

Plan: Drop this, pending feedback.

• Test-only free functions should generally live within a test_support target.

This got no real feedback.

Plan: Drop this. 

• #if defined(UNIT_TEST) is problematic and discouraged.

A few folks wanted to rip this out of the codebase.  I support that goal, but I don't want to gate on it, so I'm treating it as orthogonal to this discussion.  Dana wanted to leave this point in to try and prevent further proliferation (and noted that in general "rare" doesn't mean "would be rare without the style guide").  While I think those are valid points, it's currently difficult to even know #if defined(UNIT_TEST) is even possible, so it's unlikely people will try to do it; and the ForTesting thing above seems like it covers most of the cases where people might think of this (or removing the existing instances of these wouldn't be so possible).  So I think the risk of this proliferating if we remove the recommendation is very low.

Plan: Drop this.

 

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

There was a lot of discussion of this, but much of it covered things the rule doesn't actually talk about (e.g. multiple inheritance).  There was also no discussion (that I noticed) of why, if this is such a good thing for readability, the Google style guide doesn't do it and we need to differ.  It seems to me like our differences from Google style should be rare, and almost always be due to Chromium-specific circumstance, not just "document what we think makes code readable".  Every style rule we add to remember weakens the memorability and compliance of all the rules (hence this simplification effort).

Anyway, if we want to keep something, here's my attempt at more comprehensive wording.  See what you think:

"In class declarations, group all function overrides within each access control section, with one labeled group per parent class."

Plan: Replace with modified wording, pending feedback.

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

• Don't use else after return.

These didn't have significant support.

Plan: Drop these.

 

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

Dana noted that this is a Chrome-ism that can be hard to figure out.  I agree, and I think we should document this.  I still don't think that makes it a "style rule", however, more "something you may need to know to get your code working" (and Jeremy seems to agree).  So I still think this should be moved.  I don't love the Dos and Don'ts page as a destination, but I like it better than leaving it here.  The other option would be to put this in markdown and link it from somewhere, maybe with a comment in the code on the various files that define the export macros?

Plan: Move this, pending feedback.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

These didn't have a lot of support.  Dana suggested the second should perhaps go in some tips somewhere, and there was a tangential discussion about constant initialization.  I think this second bullet is more obvious than a lot of the existing stuff in the Dos and Don'ts page, so I think it's OK to just drop these.  The other part of my reaction is cynicism: I've committed many dozens of CLs to fix incorrect size_t usage, and people still get it wrong all the time (including senior people who will defend their behavior if called on it), so I'm skeptical of the value of any of our existing wording :(

Plan: Drop these.  

Also (and sorry for throwing this in at a late stage), I think we should drop this bullet as redundant with the Google style guide:

• In cases where the exact size of the type matters (e.g. a 32-bit pixel value, a bitmask, or a counter that has to be a particular width), use one of the sized types from <stdint.h>, e.g. uint32_t.  
  

Plan: Drop this, pending feedback.

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)

Nico and Dana both suggested keeping this.

Plan: Keep this.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

Dana noted that as an extension to a style rule, this probably belongs in the style guide itself.  Fair enough.

Plan: Keep this.

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

Gab and Dana both noted that the content here is valuable.  I agree, I just worry that this is a ton of detail for the style guide.

Plan: Keep this.

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

+1s from Dana on these.

Plan: Do these.

PK

[Jan Wilken Dörrie]

Thanks a lot, Peter! This LGTM, I just have a minor comment below (exporting symbols section).

Early this year there was a discussion around this on chromium-dev, out of which the COMPONENT_EXPORT macro and base/component_export.h was born. This obsoletes the various FOO_EXPORT macros, which should be replaced by COMPONENT_EXPORT(FOO). All of this, including the deprecated method, is documented here: https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md 

Thus I vote to simply remove this section from the style guide, make an effort to replace the various FOO_EXPORTs we still have, and add a link to docs/component_build.md in base/component_export.h. 

Best regards,

Jan

 

--
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 post to this group, send email to c...@chromium.org.

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

--

Jan Wilken Dörrie

Software Engineer

jdoe...@google.com
 +49 89 839300973

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

    

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Nico Weber]

This is is a bit of a thread derail, but the new component_export.h isn't universally loved, e.g. https://chromium-review.googlesource.com/c/chromium/src/+/1259242/3/ui/gfx/geometry_skia_export.h#1

 

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

[Peter Kasting]

On Thu, Nov 22, 2018 at 6:27 AM Nico Weber <tha...@chromium.org> wrote:

On Thu, Nov 22, 2018 at 3:46 AM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:

Early this year there was a discussion around this on chromium-dev, out of which the COMPONENT_EXPORT macro and base/component_export.h was born. This obsoletes the various FOO_EXPORT macros, which should be replaced by COMPONENT_EXPORT(FOO). All of this, including the deprecated method, is documented here: https://chromium.googlesource.com/chromium/src/+/master/docs/component_build.md 

Thus I vote to simply remove this section from the style guide, make an effort to replace the various FOO_EXPORTs we still have, and add a link to docs/component_build.md in base/component_export.h. 

This is is a bit of a thread derail, but the new component_export.h isn't universally loved, e.g. https://chromium-review.googlesource.com/c/chromium/src/+/1259242/3/ui/gfx/geometry_skia_export.h#1

Can you folks start a different thread to discuss this one?  For the context of this thread, I'm not planning to change the text of this recommendation, even if it were to move.

PK

[dan...@chromium.org]

I think these are both interesting ideas but less discoverable for new folks. I'd still vote to keep this where it is until a better place could be found.

 

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

These didn't have a lot of support.  Dana suggested the second should perhaps go in some tips somewhere, and there was a tangential discussion about constant initialization.  I think this second bullet is more obvious than a lot of the existing stuff in the Dos and Don'ts page, so I think it's OK to just drop these.  The other part of my reaction is cynicism: I've committed many dozens of CLs to fix incorrect size_t usage, and people still get it wrong all the time (including senior people who will defend their behavior if called on it), so I'm skeptical of the value of any of our existing wording :(

Perhaps the wording could be improved, or citations could be added to more details? If it's not useful enough to link to in situations where this comes up, then maybe it's worth investing in some better wording, since it sounds like it comes up often.

 

Plan: Drop these.  

Also (and sorry for throwing this in at a late stage), I think we should drop this bullet as redundant with the Google style guide:

• In cases where the exact size of the type matters (e.g. a 32-bit pixel value, a bitmask, or a counter that has to be a particular width), use one of the sized types from <stdint.h>, e.g. uint32_t.  
  

Plan: Drop this, pending feedback.

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)

Nico and Dana both suggested keeping this.

Plan: Keep this.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

Dana noted that as an extension to a style rule, this probably belongs in the style guide itself.  Fair enough.

Plan: Keep this.

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

Gab and Dana both noted that the content here is valuable.  I agree, I just worry that this is a ton of detail for the style guide.

Plan: Keep this.

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

+1s from Dana on these.

Plan: Do these.

PK

--

[Peter Kasting]

On Mon, Nov 26, 2018 at 8:03 AM <dan...@chromium.org> wrote:

On Wed, Nov 21, 2018 at 7:06 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

Dana noted that this is a Chrome-ism that can be hard to figure out.  I agree, and I think we should document this.  I still don't think that makes it a "style rule", however, more "something you may need to know to get your code working" (and Jeremy seems to agree).  So I still think this should be moved.  I don't love the Dos and Don'ts page as a destination, but I like it better than leaving it here.  The other option would be to put this in markdown and link it from somewhere, maybe with a comment in the code on the various files that define the export macros?

Plan: Move this, pending feedback.

I think these are both interesting ideas but less discoverable for new folks. I'd still vote to keep this where it is until a better place could be found.

My concern here is that this is one of many, many pieces of info you might need to know about the codebase during the course of your work, and by no means the most important.  I've needed to add export macros to things maybe three times; new people are unlikely to need it straight off, as it's only necessary when adding new classes/APIs to core headers, and only undiscoverable when doing so in a brand new header file.  Those factors select against new team members running into this.  Meanwhile, more important, widely-used, and needed-by-new folks stuff like "a guide to the geometry classes" or "all the different ways to do integral type conversions" or "what the heck are the various URL types, when do I use them, and how do I safely format them for use or display" are not in the style guide (rightfully so IMO).

To me the style guide should be "the stuff you really need to know before you write a single patch".  It should be limited to issues of style, not issues of general documentation of how to write code in Chromium.  And it should be short enough and widespread enough to be memorized by every author and reviewer in Chrome, because everyone is expected to uphold it in every review.  So every word matters (and there are a lot of words in this section).

If those are reasonable limitations to place on the style guide, then I think this clearly should go elsewhere on dev.chromium.org, along with those other things I mentioned and many more.  Having comprehensive, searchable, organized, up-to-date documentation would be a great boon (and a huge amount of work).  But at least for starters, we could move this to a page linked from http://dev.chromium.org/developers/how-tos , or agree to deprecate the individual EXPORT macros in favor of the component export stuff and put this in markdown linked from that header file.  I'm willing to do either of those.  I just don't want to leave this here -- I think it has large cost and small benefit in this particular location.

Maybe I'm way off.  Other people can feel free to weigh in.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

These didn't have a lot of support.  Dana suggested the second should perhaps go in some tips somewhere, and there was a tangential discussion about constant initialization.  I think this second bullet is more obvious than a lot of the existing stuff in the Dos and Don'ts page, so I think it's OK to just drop these.  The other part of my reaction is cynicism: I've committed many dozens of CLs to fix incorrect size_t usage, and people still get it wrong all the time (including senior people who will defend their behavior if called on it), so I'm skeptical of the value of any of our existing wording :(

Perhaps the wording could be improved, or citations could be added to more details? If it's not useful enough to link to in situations where this comes up, then maybe it's worth investing in some better wording, since it sounds like it comes up often.

We're still leaving a variety of other bullets; I think the value of these particular ones is very low.  The only thing I'd occasionally want to tell people is that there's a semantic difference between e.g. size_t and uint64_t, but that's really already covered by the stuff we're leaving in.

As to whether there is better wording for the bullets that are being left in... perhaps?  I don't know how much more clearly this stuff can be stated.  I'll leave that for if someone has a specific improvement proposal, maybe in a separate thread.

PK

[dan...@chromium.org]

On Mon, Nov 26, 2018 at 2:17 PM Peter Kasting <pkas...@google.com> wrote:

On Mon, Nov 26, 2018 at 8:03 AM <dan...@chromium.org> wrote:

On Wed, Nov 21, 2018 at 7:06 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

Dana noted that this is a Chrome-ism that can be hard to figure out.  I agree, and I think we should document this.  I still don't think that makes it a "style rule", however, more "something you may need to know to get your code working" (and Jeremy seems to agree).  So I still think this should be moved.  I don't love the Dos and Don'ts page as a destination, but I like it better than leaving it here.  The other option would be to put this in markdown and link it from somewhere, maybe with a comment in the code on the various files that define the export macros?

Plan: Move this, pending feedback.

I think these are both interesting ideas but less discoverable for new folks. I'd still vote to keep this where it is until a better place could be found.

My concern here is that this is one of many, many pieces of info you might need to know about the codebase during the course of your work, and by no means the most important.  I've needed to add export macros to things maybe three times;

This was surprising but maybe related to the area of the code we each work in. Every class in most parts of chrome outside of content/ (and chrome/ ?) require export in order for the unit tests to use them. content/ does special build things in component build, though so avoids it FBOFW.

 

new people are unlikely to need it straight off, as it's only necessary when adding new classes/APIs to core headers,

I'm not sure where "core headers" comes into it, as every class used by unit tests needs to be exported. But this might be a content vs elsewhere thing.

 

and only undiscoverable when doing so in a brand new header file.

New people are also very likely to be using a static build and not run into this problem until it reaches weird linking errors on debug bots.

 

  Those factors select against new team members running into this.  Meanwhile, more important, widely-used, and needed-by-new folks stuff like "a guide to the geometry classes" or "all the different ways to do integral type conversions" or "what the heck are the various URL types, when do I use them, and how do I safely format them for use or display" are not in the style guide (rightfully so IMO).

Agreed as these are about the types in our code base/structure, not about how to write code correctly for our expectations and toolchain. (Maybe the latter is where you feel like the style guide doesn't fit anymore, and I differ.)

To me the style guide should be "the stuff you really need to know before you write a single patch".  It should be limited to issues of style, not issues of general documentation of how to write code in Chromium.  And it should be short enough and widespread enough to be memorized by every author and reviewer in Chrome, because everyone is expected to uphold it in every review.  So every word matters (and there are a lot of words in this section).

Philosophically, I'm not sure this lines up with the Google Style Guide. It's a huge document that covers all corner cases of using C++. Many rules won't matter when you're writing your first CL, or 50th CL until you come across that scenario, similar to adding a new class, don't you think? For example https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors doesn't come up until writing a constructor.

If those are reasonable limitations to place on the style guide, then I think this clearly should go elsewhere on dev.chromium.org, along with those other things I mentioned and many more.  Having comprehensive, searchable, organized, up-to-date documentation would be a great boon (and a huge amount of work).  But at least for starters, we could move this to a page linked from http://dev.chromium.org/developers/how-tos , or agree to deprecate the individual EXPORT macros in favor of the component export stuff and put this in markdown linked from that header file.  I'm willing to do either of those.  I just don't want to leave this here -- I think it has large cost and small benefit in this particular location.

If we do the COMPONENT() thing I'd still appreciate a rule saying to use it (and link to the markdown). Another point is we really don't want any one-off divergence in terms of how this is done, as code is added/imported into chromium over time.

Maybe I'm way off.  Other people can feel free to weigh in.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

These didn't have a lot of support.  Dana suggested the second should perhaps go in some tips somewhere, and there was a tangential discussion about constant initialization.  I think this second bullet is more obvious than a lot of the existing stuff in the Dos and Don'ts page, so I think it's OK to just drop these.  The other part of my reaction is cynicism: I've committed many dozens of CLs to fix incorrect size_t usage, and people still get it wrong all the time (including senior people who will defend their behavior if called on it), so I'm skeptical of the value of any of our existing wording :(

Perhaps the wording could be improved, or citations could be added to more details? If it's not useful enough to link to in situations where this comes up, then maybe it's worth investing in some better wording, since it sounds like it comes up often.

We're still leaving a variety of other bullets; I think the value of these particular ones is very low.  The only thing I'd occasionally want to tell people is that there's a semantic difference between e.g. size_t and uint64_t, but that's really already covered by the stuff we're leaving in.

Yeah ok, agree.

[Peter Kasting]

On Mon, Nov 26, 2018 at 1:10 PM <dan...@chromium.org> wrote:

On Mon, Nov 26, 2018 at 2:17 PM Peter Kasting <pkas...@google.com> wrote:

On Mon, Nov 26, 2018 at 8:03 AM <dan...@chromium.org> wrote:

On Wed, Nov 21, 2018 at 7:06 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

Dana noted that this is a Chrome-ism that can be hard to figure out.  I agree, and I think we should document this.  I still don't think that makes it a "style rule", however, more "something you may need to know to get your code working" (and Jeremy seems to agree).  So I still think this should be moved.  I don't love the Dos and Don'ts page as a destination, but I like it better than leaving it here.  The other option would be to put this in markdown and link it from somewhere, maybe with a comment in the code on the various files that define the export macros?

Plan: Move this, pending feedback.

I think these are both interesting ideas but less discoverable for new folks. I'd still vote to keep this where it is until a better place could be found.

My concern here is that this is one of many, many pieces of info you might need to know about the codebase during the course of your work, and by no means the most important.  I've needed to add export macros to things maybe three times;

This was surprising but maybe related to the area of the code we each work in. Every class in most parts of chrome outside of content/ (and chrome/ ?) require export in order for the unit tests to use them. content/ does special build things in component build, though so avoids it FBOFW.

I had no idea this was used so commonly outside content/ and chrome/.  (I thought it wasn't needed for components/ unittests either but maybe I'm wrong.)

That elevates the degree to which I think people should be told about this, but I still don't think this goes in the style guide.  We have some onboarding docs, maybe this should go there?

Seems like we have two different axes -- whether this is important for people to find out, and whether it's about style specifically.  I'm claiming "not style and thus doesn't go here no matter how important", but it's not clear to me how much you (or others) agree.  You've convinced me that there's a meaningful population for which "important" is true (though it's a subpopoulation rather than everyone -- the number of folks working on chrome/ and content/ is still pretty large).

To me the style guide should be "the stuff you really need to know before you write a single patch".  It should be limited to issues of style, not issues of general documentation of how to write code in Chromium.  And it should be short enough and widespread enough to be memorized by every author and reviewer in Chrome, because everyone is expected to uphold it in every review.  So every word matters (and there are a lot of words in this section).

Philosophically, I'm not sure this lines up with the Google Style Guide. It's a huge document that covers all corner cases of using C++. Many rules won't matter when you're writing your first CL, or 50th CL until you come across that scenario, similar to adding a new class, don't you think? For example https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors doesn't come up until writing a constructor.

Here's the style guide's comment on its philosophy:

"Style rules should pull their weight

The benefit of a style rule must be large enough to justify asking all of our engineers to remember it. The benefit is measured relative to the codebase we would get without the rule, so a rule against a very harmful practice may still have a small benefit if people are unlikely to do it anyway. This principle mostly explains the rules we don’t have, rather than the rules we do: for example, goto contravenes many of the following principles, but is already vanishingly rare, so the Style Guide doesn’t discuss it."

I don't know whether the Google style guide manages to uphold this philosophy, but perhaps that helps guide us here?

PK

[dan...@chromium.org]

On Tue, Nov 27, 2018 at 7:39 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Mon, Nov 26, 2018 at 1:10 PM <dan...@chromium.org> wrote:

On Mon, Nov 26, 2018 at 2:17 PM Peter Kasting <pkas...@google.com> wrote:

On Mon, Nov 26, 2018 at 8:03 AM <dan...@chromium.org> wrote:

On Wed, Nov 21, 2018 at 7:06 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

Dana noted that this is a Chrome-ism that can be hard to figure out.  I agree, and I think we should document this.  I still don't think that makes it a "style rule", however, more "something you may need to know to get your code working" (and Jeremy seems to agree).  So I still think this should be moved.  I don't love the Dos and Don'ts page as a destination, but I like it better than leaving it here.  The other option would be to put this in markdown and link it from somewhere, maybe with a comment in the code on the various files that define the export macros?

Plan: Move this, pending feedback.

I think these are both interesting ideas but less discoverable for new folks. I'd still vote to keep this where it is until a better place could be found.

My concern here is that this is one of many, many pieces of info you might need to know about the codebase during the course of your work, and by no means the most important.  I've needed to add export macros to things maybe three times;

This was surprising but maybe related to the area of the code we each work in. Every class in most parts of chrome outside of content/ (and chrome/ ?) require export in order for the unit tests to use them. content/ does special build things in component build, though so avoids it FBOFW.

I had no idea this was used so commonly outside content/ and chrome/.  (I thought it wasn't needed for components/ unittests either but maybe I'm wrong.)

That elevates the degree to which I think people should be told about this, but I still don't think this goes in the style guide.  We have some onboarding docs, maybe this should go there?

Seems like we have two different axes -- whether this is important for people to find out, and whether it's about style specifically.  I'm claiming "not style and thus doesn't go here no matter how important", but it's not clear to me how much you (or others) agree.  You've convinced me that there's a meaningful population for which "important" is true (though it's a subpopoulation rather than everyone -- the number of folks working on chrome/ and content/ is still pretty large).

Yeah to me this feels like "style" in that it's a rule that must be followed and is part of how you write correct C++ for our project. In the Google style guide it even mentions similar under Windows Code, though says to use DLLIMPORT/DLLEXPORT. So I think you could argue this is an extension of the Google style guide even.

 

To me the style guide should be "the stuff you really need to know before you write a single patch".  It should be limited to issues of style, not issues of general documentation of how to write code in Chromium.  And it should be short enough and widespread enough to be memorized by every author and reviewer in Chrome, because everyone is expected to uphold it in every review.  So every word matters (and there are a lot of words in this section).

Philosophically, I'm not sure this lines up with the Google Style Guide. It's a huge document that covers all corner cases of using C++. Many rules won't matter when you're writing your first CL, or 50th CL until you come across that scenario, similar to adding a new class, don't you think? For example https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors doesn't come up until writing a constructor.

Here's the style guide's comment on its philosophy:

"Style rules should pull their weight

The benefit of a style rule must be large enough to justify asking all of our engineers to remember it. The benefit is measured relative to the codebase we would get without the rule, so a rule against a very harmful practice may still have a small benefit if people are unlikely to do it anyway. This principle mostly explains the rules we don’t have, rather than the rules we do: for example, goto contravenes many of the following principles, but is already vanishingly rare, so the Style Guide doesn’t discuss it."

I don't know whether the Google style guide manages to uphold this philosophy, but perhaps that helps guide us here?

This does I think fall under all our engineers should remember it (granted there are parts of the code that make this one thing less common but I'm sure that's true for other rules..?). It's a little different in that if you don't use it.. well it won't build in some situations. I don't know that that is a bar for the guide though.

I could see this going into the onboarding docs instead if you continue to feel strongly. It doesn't feel out of place in the style guide to me though, and maybe the example above will give you the same feeling.

 

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 post to this group, send email to c...@chromium.org.

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

[dan...@chromium.org]

On Fri, Nov 9, 2018 at 8:04 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Style arbiters,

As part of Chrome's diversity & inclusion efforts, I'm trying to make onboarding for new team members easier.  One way to help this a bit is to simplify the Chromium style guide; the fewer rules new team members have to remember and follow, the easier it is to ramp up.

Accordingly, I'd like to propose a lot of simplifications.  I could split these into separate threads, but I'm hoping one thread is easier to handle.  Let me know if I'm wrong :)

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

• Test-only free functions should generally live within a test_support target.

• #if defined(UNIT_TEST) is problematic and discouraged.

The first bullet has some value.  The second is a combination of obvious and overly-pedantic.  The third is obscure and unimportant.  The fourth is rarely attempted and probably doesn't need to be stated.

I don't think the value of the first bullet is sufficient to keep this section.

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

• Don't use else after return.

If we remove this we'll need to add it to the blink style guide, or remove the PRESUBMIT check there. I'd prefer to remove it from the PRESUBMIT myself as I want less divergence. I'm not actually sure how we'd decide to do that at this point, since the original deviations were hard choices and compromises.

** Presubmit ERRORS **

check_blink_style.py failed

  third_party/blink/renderer/core/page/page_visibility_state.cc:36:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]

 

These are all pedantic (save the bit about using the override specifier, which I think our tools enforce).  Their benefit doesn't justify forcing people to learn and abide by them.

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

...or elsewhere; this is more a "how to" than a style rule and doesn't belong in this file.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)
  

The first is implicit in common usage, and if people really violated it, fine.  The second isn't really a style rule.  The third is redundant with the Google style guide.  The fourth is hopefully unnecessary at this point with wstring being rare across the codebase.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.
  

The first sentence is redundant with the Google style guide.  The rest is more "how to do this well" than "style rule".

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

I'm not convinced this is "style rule" as much as "good advice".  I'm less sure of this recommendation.

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

These are the oldest sections in the doc and are very rambly (or in some cases obvious).

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

This may well be interesting, but it detracts from "what should I be doing".

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 post to this group, send email to c...@chromium.org.

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

[Peter Kasting]

On Wed, Nov 28, 2018 at 10:08 AM <dan...@chromium.org> wrote:

On Tue, Nov 27, 2018 at 7:39 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

I had no idea this was used so commonly outside content/ and chrome/.  (I thought it wasn't needed for components/ unittests either but maybe I'm wrong.)

That elevates the degree to which I think people should be told about this, but I still don't think this goes in the style guide.  We have some onboarding docs, maybe this should go there?

Seems like we have two different axes -- whether this is important for people to find out, and whether it's about style specifically.  I'm claiming "not style and thus doesn't go here no matter how important", but it's not clear to me how much you (or others) agree.  You've convinced me that there's a meaningful population for which "important" is true (though it's a subpopoulation rather than everyone -- the number of folks working on chrome/ and content/ is still pretty large).

Yeah to me this feels like "style" in that it's a rule that must be followed and is part of how you write correct C++ for our project. In the Google style guide it even mentions similar under Windows Code, though says to use DLLIMPORT/DLLEXPORT. So I think you could argue this is an extension of the Google style guide even.

Well... I read that section as giving this as a passing example of using macros to wrap nonstandard extensions rather than saying when or how you should actually use said extensions.  So I don't really buy your argument there.

But I still feel like I'd like to get more perspectives to triangulate between us.  Does anyone else have an opinion?  If not, I think I need to write up what it would look like to move this into some onboarding doc so as to have a concrete comparison proposal to discuss.

PK

[Vladimir Levin]

This specific section seems to have two pieces of information: 

1. What exporting does, with examples of some functions being available outside of the dll and some not being available, and

2. What to name the export macro and where to place the macro in the code (between class and ClassName, etc).

My interpretation is that 2. belongs in style, but 1. does not. My bar for this judgment is the following: If my code does not compile or does not run, then I shouldn't need to look in a style guide for an explanation why. If my code works just fine but I'm unsure about the best way to name something or how to format it or where to place it, then I should read the style guide.

It's possible to leave a small bit (2.) in the style guide and move the rest (1.) to the Dos and Don'ts... If that's not a good option, then I'd prefer to have this section somewhere outside of the style guide, naming and all.

Thanks for reading,

Vlad

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 post to this group, send email to c...@chromium.org.

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

[Jeremy Roman]

On Wed, Dec 5, 2018 at 11:34 AM Vladimir Levin <vmp...@chromium.org> wrote:

On Tue, Dec 4, 2018 at 10:25 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Wed, Nov 28, 2018 at 10:08 AM <dan...@chromium.org> wrote:

On Tue, Nov 27, 2018 at 7:39 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

I had no idea this was used so commonly outside content/ and chrome/.  (I thought it wasn't needed for components/ unittests either but maybe I'm wrong.)

That elevates the degree to which I think people should be told about this, but I still don't think this goes in the style guide.  We have some onboarding docs, maybe this should go there?

Seems like we have two different axes -- whether this is important for people to find out, and whether it's about style specifically.  I'm claiming "not style and thus doesn't go here no matter how important", but it's not clear to me how much you (or others) agree.  You've convinced me that there's a meaningful population for which "important" is true (though it's a subpopoulation rather than everyone -- the number of folks working on chrome/ and content/ is still pretty large).

Yeah to me this feels like "style" in that it's a rule that must be followed and is part of how you write correct C++ for our project. In the Google style guide it even mentions similar under Windows Code, though says to use DLLIMPORT/DLLEXPORT. So I think you could argue this is an extension of the Google style guide even.

Well... I read that section as giving this as a passing example of using macros to wrap nonstandard extensions rather than saying when or how you should actually use said extensions.  So I don't really buy your argument there.

But I still feel like I'd like to get more perspectives to triangulate between us.  Does anyone else have an opinion?  If not, I think I need to write up what it would look like to move this into some onboarding doc so as to have a concrete comparison proposal to discuss.

I'd agree info about export macros is probably technically useful information but not part of the style guide, strictly speaking. I wouldn't object to moving it, though I'm not sure I'm convinced it makes a massive difference (as both documents will still probably be worth reading). But I don't feel strongly on this point, which is why I hadn't commented earlier.

This specific section seems to have two pieces of information: 

1. What exporting does, with examples of some functions being available outside of the dll and some not being available, and

2. What to name the export macro and where to place the macro in the code (between class and ClassName, etc).

My interpretation is that 2. belongs in style, but 1. does not. My bar for this judgment is the following: If my code does not compile or does not run, then I shouldn't need to look in a style guide for an explanation why. If my code works just fine but I'm unsure about the best way to name something or how to format it or where to place it, then I should read the style guide.

It's possible to leave a small bit (2.) in the style guide and move the rest (1.) to the Dos and Don'ts... If that's not a good option, then I'd prefer to have this section somewhere outside of the style guide, naming and all.

Thanks for reading,

Vlad

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 post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAa%2B3zV6V49c6Mx0N1SmokMuoMqHzpsbJq6j3Q34YyXUQ%40mail.gmail.com.

--
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 post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CADsXd2MMq6Dt%3DWyh0uK%2B7hs-5ZeyD-DGUT6MYwzkrDg8eA9DLw%40mail.gmail.com.

[Nico Weber]

(Maybe the agreed-upon changes could be landed without blocking that on agreement on the export macros question. That way, we get (most of) the simplifications sooner :-) )

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13cMZqZb9MUrGB%2BB-DGDJ8gBb2XzewoT3LNTUY1Urkx-MA%40mail.gmail.com.

[Peter Kasting]

On Thu, Dec 6, 2018 at 10:45 AM Nico Weber <tha...@chromium.org> wrote:

(Maybe the agreed-upon changes could be landed without blocking that on agreement on the export macros question. That way, we get (most of) the simplifications sooner :-) )

It sounds to me like there's more weight behind moving at least part of the export macros stuff elsewhere.  I have enough to move forward, it's mostly a matter of writing up the changes and then sending the proposal for final review.

That is, unless there were other comments on places where I said "pending feedback".

PK

[Peter Kasting]

I've finally uploaded a patch to implement the suggestions here, at https://chromium-review.googlesource.com/c/chromium/src/+/1606590.  Here's what I actually did:

From: Peter Kasting <pkas...@google.com>
Date: Wed, Nov 21, 2018 at 4:05 PM
To: <c...@chromium.org>

On Fri, Nov 9, 2018 at 5:04 PM Peter Kasting <pkas...@google.com> wrote:

(1) Remove "Test-only Code" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#test_only-code ), which contains this text:

• Functions used only for testing should be restricted to test-only usages with the ForTesting suffix. This is checked at presubmit time to ensure these functions are only called by test files.

Plan: Keep this bullet in the style guide.

Kept.

• Test-only constructors cannot have the ForTesting suffix. Instead, they should be declared protected with a test-only subclass, or private with a test-only friend class. They should be commented as For testing only.

Plan: Drop this, pending feedback.

Dropped.

• Test-only free functions should generally live within a test_support target.

Plan: Drop this. 

Dropped. 

• #if defined(UNIT_TEST) is problematic and discouraged.

Plan: Drop this.

Dropped.

(2) Remove the following bullets from the "Code formatting" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting ):

• When you derive from a base class, group any overriding functions in your header file in one labeled section. Use the override specifier on all these functions.

"In class declarations, group all function overrides within each access control section, with one labeled group per parent class."

Plan: Replace with modified wording, pending feedback.

Replaced with above wording.

• Prefer putting delegate classes in their own header files. Implementors of the delegate interface will often be included elsewhere, which will often cause more coupling with the header of the main class.
  

• Don't use else after return.

These didn't have significant support.

Plan: Drop these.

Dropped.  Removed Blink PRESUBMIT check.

(3) Move the "Exporting symbols" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#exporting-symbols ) to the "Dos and Don'ts" page.

Dana noted that this is a Chrome-ism that can be hard to figure out.  I agree, and I think we should document this.  I still don't think that makes it a "style rule", however, more "something you may need to know to get your code working" (and Jeremy seems to agree).  So I still think this should be moved.  I don't love the Dos and Don'ts page as a destination, but I like it better than leaving it here.  The other option would be to put this in markdown and link it from somewhere, maybe with a comment in the code on the various files that define the export macros?

Plan: Move this, pending feedback.

Condensed to just the "style" bits by removing explanation about what exporting does.  This change had the most discussion; hopefully this route is satisfactory.

(4) Remove the following bullets from the "Types" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types ):

• Use size_t directly in preference to std::string::size_type and similar.

• Be aware that size_t (object sizes and indices), off_t (file offsets), ptrdiff_t (the difference between two pointer values), intptr_t (an integer type large enough to hold the value of a pointer), uint32_t, uint64_t, and so on are not necessarily the same. Use the right type for your purpose.
  

• Do not use unsigned types to mean “this value should never be < 0”. For that, use assertions or run-time checks (as appropriate).
  

Plan: Drop these.

Dropped, and condensed nearby wording.

• In cases where the exact size of the type matters (e.g. a 32-bit pixel value, a bitmask, or a counter that has to be a particular width), use one of the sized types from <stdint.h>, e.g. uint32_t.  
  

Plan: Drop this, pending feedback.

Dropped.

 

• Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.)

Plan: Keep this.

Kept.

(5) Move the following bullet from the "Types" section to the "Dos and Don'ts" page:

• Follow Google C++ casting conventions to convert arithmetic types when you know the conversion is safe. Use checked_cast<>() (from base/numerics/safe_conversions.h) when you need to enforce via CHECK() that the source value is in-range for the destination type. Use saturated_cast<>() (from the same file) if you instead wish to clamp out-of-range values.

Plan: Keep this.

Kept.

(6) Consider moving the "Object ownership and calling conventions" section ( https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions ) to the "Dos and Don'ts" page.

Plan: Keep this.

Kept.

(7) In the "Dos and Don'ts" page, rewrite sections 2 and 3 to be brief and imperative.

(8) In the "Dos and Don'ts" section on const ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly ), move everything but the TLDR to a separate document.

Plan: Do these.

Done both.

Also, removed the section on the advantages of =default (note: not the recommendation itself, just the justification) and removed a few other bits of justification wording since a prescriptive guide generally doesn't need to give detailed rationale.

PK

[Gabriel Charette]

Thanks

FWIW, I like having rationale. Otherwise it's hard to justify linking to it in a CR.

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/CAAHOzFDdHu%3DdRByd74vTTmG%3Drr1ONRjL39TbqRp%3Dww0FAyOj6A%40mail.gmail.com.