> Although GSG does not forbid them categoricaly, it says to “Avoid using forward declarations where possible”. However forward declarations are useful in contexts like the following: ...
Yes, those are the cases where it isn't possible to avoid forward declarations, so it's fine to use them?
> While it’s true that initialization/destruction order of globals between translation units is not defined and can pose problems, this rule is overly prohibitve. Globals like this are not problematic and very useful:
No, globals like that are very problematic, so much so that there is a clang warning specifically to detect these (-Wexit-time-destructors). exit runs global destructors, so if one thread calls exit while some other thread is still doing stuff, it might destroy an object that's in use, leading to explosions. Also, if you're exiting, why are you paying to deallocate memory when it's all going to be blown away anyway?
> The lynchpin of C++ are value-types. Such types should be copyable and moveable and the language automatically generates the necessary constructors and operators by default. “a copyable class should explicitly declare the copy operations, a move-only class should explicitly declare the move operations” – this goes against the language philosophy.
It is very non-obvious which constructors will get automatically generated, because it depends on the types of the members. `Foo(Foo&& move) = default;` makes it immediately obvious.
> in other words, the reader is expected to understand the semantics of an unknown function without consulting the declaration (or documentation).
The idea that the author considers making code as self-documenting as possible at the call site a conceptual mistake is just weird to me.
Indeed, they're terrible. What I don't understand is why basically every Google-authored project has loads of these (protobuf, gflags, TensorFlow...)
> GSG forbids exceptions on the ground of “Google’s existing code is not exception-tolerant”.
Unless you are doing embedded software, you should use exceptions. I have tried this "modern" trend of returning instead of raising a few times and I'm not convinced at all its less bloated or less error prone way of doing things.
It generates constructors and operators by default. They may not be the ones you want, though. They will satisfy the compiler, but they may do the wrong thing. If the default thing is the right thing, then yes, "= default" makes it obvious. If you don't say, then it isn't obvious whether the default is right, or the default is wrong but you missed it.
> exit runs global destructors, so if one thread calls exit while some other thread is still doing stuff, it might destroy an object that's in use, leading to explosions.
Or you can structure your code in a way that all your threads join prior to main exiting. Never call exit() or _exit() or even pthread_exit(). I think it's much cleaner. Not to mention that not every program is multi-threaded.
> It is very non-obvious which constructors will get automatically generated, because it depends on the types of the members. `Foo(Foo&& move) = default;` makes it immediately obvious.
Agreed but can you imagine what a simple
struct Point { int x, y; };
become if we were to make everything explicit?
The author explicitly mentions how it is possible to avoid forward declarations in those cases: "both of these forward declarations are possible to avoid by type-punning through a void* but it is not a good pattern."
Why even mention a non-starter choice, like using type punning through a void pointer, as an alternative to a forward declaration.
If a construct is "not possible to avoid" that generally means "not possible to avoid without changing the structure of code and run-time data (let alone changing them to something unsafe)".
Gratuitous forward declarations of C++ classes are "bad" for various reasons. Such as: people sometimes write them just to shut up the compiler, instead of including the right header to provide that declaration. "I'm just using a pointer to this; what's the harm."
It makes perfect sense for a coding standard to advise against this.
Those engineers often have to touch parts of the monorepo that are far away from what they usually work on, creating changes that need to go through reviews by teams that they have never interacted with before, and in turn need to be understood by future engineers in a similar position.
For example, the author states that "top level/application code" does not need to be in a namespace, but it's often downright absurd to classify what's considered "top level" code in Google's monorepo. I also chuckled at this:
> “Do not define implicit conversions”. I would urge the reader to consider what life would be like if std::string(const char*) constructor was marked explicit (especially in the absense of user defined literals, which GSG also outlaws). Nuff said.
At least when I worked with the code (many years ago, parts of Maps specifically), Google had its entirely own string handling routines that followed their conventions, and I did not miss std::string's implicit constructor much. This is a completely different world, where the standard library does not matter, and almost every code comes from Google themselves.
I agree. People work on their small pet projects for school or wherever and think that's how enterprise development really works. Unless they have worked on a major project, it's very difficult for them to understand.
But even more important than sytle, it's consistency. You shouldn't use a coding style/standard because it works for google. You should create a coding style that works for your team or organization and stick to it consistently. Oftentimes, keeping to the style is more important than the style itself.
std::vector<int> v;
v.resize(10, 42);
std::vector<int> v(10);
std::fill(v.begin(), v.end(), 42);
std::vector<int> v{42, 42, 42, 42, 42, 42, 42, 42, 42, 42};
* "Not marking it inline is a sure way to prevent inlining" is totally wrong) * The bit about using-directives is also pretty wrong. See https://abseil.io/tips/153 for why. And the example the author gives is terrible:
void foo() {
using namespace std::placeholders;
std::bind(bar, _1, 2);
void foo() {
using std::placeholders::_1;
std::bind(bar, _1, 2);
To summarize, the Google C++ Style Guide is there to assist reviewers in reviewing new code. Contrary to what the author thinks, the guide doesn't prescribe anything. At Google decisions are always left to the reviewer. Read the Abseil libraries to see how the style guide applies and when it is ignored.
```
int kDefaultValue = 42;
size_type kExpectedSize = 10;
v.resize(kExpectedSize, kDefaultValue);
```
People need to remember that just because Google does something one way does not imply it is correct nor that it is incorrect
When I started at Google out of college, I came in writing clever, elegant code, and had to learn that, in large systems that will be touched by lots of people over large periods of time, readability is often more than worth sacrificing the maximally performant, concise, or elegant system. One of the ways to maintain this across a large organization is by limiting the set of available footguns, and granting exceptions when needed (eg, for "library code"). The latter type of code absolutely is bifurcated from generally-accessible and modifiable systems. The cost of doing the latter is increasing the hurdles to requires to modify the code, which already is and should be the case for widely-used library code. This seems obvious enough to me that I'm not sure how the author is misunderstanding it so badly (either that or I am).
Now, the author isn't completely wrong: another thing I've noticed is how much cargo cult, "Google does it" behavior there is among startups. Creating such an large-org-safe subset of C++ usage may not be appropriate for every type of organization, and if you don't understand why you're using GSG, it's probably worth figuring out whether it's a good fit. But the author utterly fails to make the claim that it's "no good", or even that it isn't appropriate in plenty of situations.
> GSG prefers #ifndef/#define idiom over the simpler #pragma once. Yes, #pragma once is non-standard but widely supported.
I mean, it’s non-standard. What more do you need?
> Although GSG does not forbid them categoricaly, it says to “Avoid using forward declarations where possible”.
The example provided is like the one place you would use a forward declaration. I’m sure it’s fine in this case.
> Marking the function “inline” lets the compiler make the decision. Not marking it inline is a sure way to prevent inlining, unless Link Time Optimizations are turned on.
…for external linkage only. Inside your own code base, the compiler has this authority everywhere.
I switched our codebase after finding several variations of #ifdef message_h #define massage_h That potential bug existed in our code base for years (I checked history)
In other words, my vimrc pulls in a global vimrc that, when I open a new .cc file, it starts as a stub that has the correct ifdefs and some common imports (flags iirc). This, combined with a lint/presubmit that prevents submission of files missing the correct defs makes this a nonissue in practice.
The error could be diagnosed by the compiler---submit a patch to gcc!
That is to say, if the following pattern occurs around the content of the file:
#ifndef <whatever0>
#define <whatever1>
#endif
Note that GCC already looks for this pattern and optimizes it! That has been implemented for ages; probably twenty years if not more. See here:
https://gcc.gnu.org/onlinedocs/cpp/Once-Only-Headers.html
That optimization must have a check in place that it's the same symbol in both places. And so that piece of code which checks can probably be augmented to emit a warning fairly easily.
A sophisticated version of the warning would only kick in if the symbols appear to be similar (e.g. close by Levenschtein distance) so that one of them is plausibly a typo for the other.
Another solution is to have a commit hook which looks for such a problem: it enforces the include guard on headers, and checks that the symbols match.
so are most programming languages used in the world. In practice, #pragma once is less trouble than #ifdef ; we had the debate just today in reddit :-) https://www.reddit.com/r/cpp/comments/a14o5q/real_world_prob...
Meanwhile, the "trouble" the reddit thread comes up with is copy/paste programming for #ifdef. That's a one-time effort of writing a commit hook that checks for that. But no amount of effort will ensure the next compiler you need supports #pragma once
> Although GSG does not forbid them categoricaly, it says to “Avoid using forward declarations where possible”. However forward declarations are useful in contexts like the following [...]
Yes, circular data structures are a great reason to use a forward declaration; probably the best one. I think it is a perfect example of why the recommendation against forward declarations is not a prohibition.
Please, please do not forward-declare types you do not own. It constraints the way those types can be refactored. More info here: https://abseil.io/about/compatibility#what-users-must-and-mu...
> The rule basically says that global/static variables with non-trivial constructors and destructors are not allowed. While it’s true that initialization/destruction order of globals between translation units is not defined and can pose problems, this rule is overly prohibitve. Globals like this are not problematic and very useful:
A safe alternative to this is to use a static object inside a function. This can serve the same purpose without the gotchas.
> Even the inter-translation unit ordering is not a huge problem – Stroustrup discusses it in his D&E book.
Experience says otherwise. In the protobuf code-base we have to put a thread-safe initialization check in all of our constructors because we have to statically initialize our default instances, but we can't guarantee these static initializers will run before other static initializers that use them. Static initialization ordering is a real and difficult problem.
Read "Exceptional C++". It isn't worth trying to support them. It is an exercise in masochism. It isn't at all like tabs-vs-spaces.
Exceptions can work ok in a GCed language, but I haven't seen them work well otherwise. Maybe it is possible, but in C++ it is a huge trap and isn't worth the extra care that will have to go into everything to avoid really bad problems.
This is presented without justification as if it must be true. But is it?
My view is that you absolutely should have different worlds. You can then limit the number of people and the amount of code using foot gun prone constructs to a very small minority, and instead provide higher level abstractions. I'd absolutely hate to see code that looks like Boost on the business layer of an application.
Talk about an appeal to authority!
Cppcon had an entire talk titled “The Nightmare of Initialization in C++”. C++ initialization is indeed a total clusterfuck. And that’s not a controversial statement!
I’m sure each thing was added to solve a problem. But now there are too many things, and that’s the new problem.
* "Don't use forward declarations unless necessary" with the counter example of why it's bad, being a case we're they're necessary?
* The complaint against "inline" is nonsense. The only thing inline impacts is function linkage. Literally nothing else. If the compiler thinks a function is profitable to inline, it will inline it. If it does not think inlining is profitable it won't, the fact that you say "inline" means nothing.
* Complaining about complex globals initialisers: they directly impact library load time, they do cause breakages on ordering. Yes you could structure your code to avoid ordering issues, but then every developer needs to work for all time to avoid breaking it. Or you could just not use them, and it's not a problem.
* Restricting copies: It should be super easy to tell at a glance if code is going to be needlessly inefficient. Easiest way to ensure that is to make your code fail to compile.
* Exceptions: they add significantly to code size and launch time. The failure modes are hard to reason about. The standard idiom everyone is move to is option<> or result<> types. It means you have to handle errors, or be explicit in ignoring them.
* Template meta programming is extremely slow to compile, and very difficult for people to understand. constexpr is easier to read and understand, more efficient to compile, and can also be reused for non-constant cases.
* Boost: if you're using boost then you're requiring all your developers to install boost. Then you need to also ensure that they're all using the same version, and then people can end up requiring multiple versions. Again, super large company, with many many engineers and many projects mean the chances of multiple projects depending on different versions of libraries will cause misery.
* C++11 - Cool, it's 2018, guess what: many machines are running versions of an OS that has a 2018 edition of the C++ standard library. If you compile targeting newer versions of the standard library then you can't ship to those systems, unless you include a copy of the standard library in your binary. Then you have to hope your copy of the standard library doesn't conflict with any loaded by the system's own libraries.
C++11 - Cool, it's 2018, guess what: Migrating a codebase as large as Google's from C++11 to C++1x is a bunch of work. And a lot of the C++1x goodies are library additions, available in Abseil <https://github.com/abseil/abseil-cpp>.
That's not actually true. At least in clang it lowers the threshold at which a function will become inlined. You can see that happening in this wonderful demonstration by Jason Turner: https://youtu.be/GldFtXZkgYo
"The intent of this document is to provide maximal guidance with reasonable restriction. As always, common sense and good taste should prevail."
There are good arguments for deriving from the guidelines in lots of situations, guidelines merely establish the default style, but don't prohibit code that does not follow the guidelines _if_ there is a good reason to. What the guidelines establish is a duty to justify deviations.
They don't really try and understand the rationale in detail and then argue that that rationale would be better served through a different set of features, they mostly just say "hey I think Google is wrong and these features are pretty cool"
Hire developers who voluntarily follow best practices and use good judgment...don’t try to mandate it with an arbitrary list of forbidden practices.
Google is right here; a purely build-time issue, like avoiding including a header file twice, should be solved without having to resort to nonstandard language features.
The generated code doesn't change in any way, so it is gratuitous.
Save the language extensions for doing something that is inherently nonportable, like gaining access to some platform feature.
The detailed semantics of #pragma once is murky. If #pragma once appears in a file, will that preclude an identical copy of the file from being included? Do we go by content, or just some filesystem identifier like device and inode number? Or the absolute path? Will #pragma once preclude that same file from being included again through a symbolic or hard link? Is the answer the same for all compilers?
The semantics of the #ifndef/#define/#endif trick is crystal clear; we can predict what it will do in any given situation. The identity of the file, for the purpose of suppressing duplicates, is tied to a made-up symbol which the program explicitly specifies, and that's that. It may go wrong (e.g. due to clashes on the identifier between unrelated files), but in a way that is understandable.
#pragma once seems like a bad trade-off for the sake of saving two lines of preprocessing.
As usual, the devil lies in the detail. If you want to make pragma once robust you need to checksum files, which in the end will be slower than include guards.
Uhm… what?
1) I would challenge the author to produce any useful use of anonymous namespaces in header files that doesn't violate ODR. I'd also challenge them to not break ODR with constants in header files in the way they seem to mean.
I should add: without UB, plz.
2) How else do we declare constants? Uhm, how about: extern const int foo; or just "inline constexpr int foo = 1" in header but non-static and not in anonymous namespace?
> Globals like this are not problematic and very useful: [example of nontrivially destructible global]
It doesn't look dangerous, but allowing it does not scale to a billion lines of code. Code easier to reason about is less buggy code.
In general though: Author spends 1 SWE-year per year on coding, Google spends what, 100 SWE-millenia per year? It's optimizing for that use case.
> Unlike C++ Core Guidelines that try to explain how to use the language effectively, GSG is about forbidding the use of certain features.
They aren't meant to accomplish the same thing. The GSG's goal is to standardize both style and feature set across the mono repo so that thousands of engineers can effectively contribute.
> We should not bifurcate developers into two camps as the reality exists in a continuum between these two extremes.
It's a style guide, not an unwavering book of law. Library writers use some features that aren't necessary in non-library code. In fact, some language features are written specifically to the benefit of the implementers.
> GSG prefers #ifndef/#define idiom over the simpler #pragma once. Yes, #pragma once is non-standard but widely supported.
When the difference is a 2 lines of code that you probably have tooling to generate anyways, why not default to the thing that's standard?
> Yes, both of these forward declarations are possible to avoid by type-punning through a void* but it is not a good pattern.
The point of "Avoid using fwd-declarations where possible" is exactly that: reason about the code and whether the declaration is necessary. It's not "dogmatically avoid forward declarations". I'm sure no-one at Google is punning through void* for those examples.
> Marking the function “inline” lets the compiler make the decision. Not marking it inline is a sure way to prevent inlining, unless Link Time Optimizations are turned on.
That's simply not true. The compiler can inline a function if it can infer that it doesn't alter the program's behavior. Generally, tools make good decisions. Override them when you've measured that something else is better.
> Library code should always be placed in a namespace. Top level/application code has questionable value being placed in a namespace.
It's easier to be diligent about placing everything in a namespace because application code doesn't always remain so. Especially not in a gigantic mono repo like Google's.
> GSG prohibits the use of static definitions and annonymous namespaces in header files. How else do we declare constants?
I mean, you declare your constants in the header and define them in the implementation file. First example I can find in Chromium: https://cs.chromium.org/chromium/src/ios/chrome/browser/pref...
> The rule basically says that global/static variables with non-trivial constructors and destructors are not allowed. While it’s true that initialization/destruction order of globals between translation units is not defined and can pose problems, this rule is overly prohibitve.
This rule isn't overly prohibitive in the context of large applications that require clean startups/shutdowns and contain objects that are sensitive to construction/destruction order.
> “Do not define implicit conversions”. I would urge the reader to consider what life would be like if std::string(const char) constructor was marked explicit (especially in the absense of user defined literals, which GSG also outlaws). Nuff said.
The issue with implicit conversions is that they're not explicit to callers. Honestly, having to wrap a few const char* into std::string() calls wouldn't be as bad as you seem to think it would be. Explicit code is easier to read and reason about.
> “a copyable class should explicitly declare the copy operations, a move-only class should explicitly declare the move operations” – this goes against the language philosophy.
Again, this is about being explicit, and using safe defaults. It's not prohibited to make things copyable and movable but defaulting to the most restrictive set up and adding things as needed ensures that developers thought about the implications of those properties.
> In this pattern the private base cannot be replaced by a data member because the base is constructed before the members.
I've never seen code with private inheritance that made sense and couldn't be refactored to something clearer.
> Operator Overloading
See my point about implicit conversion above because it's the same thing here. Operator Overloading tends to obfuscate code.
> I think this rule mixes together two ideas for no good reason. C++ has a clear way to denote mutability – the const qualifier. Using pointer vs reference to signal mutability goes against the language.
The const qualifier isn't visible from the call site. This rule makes it so that you can reason about what will happen to the values you're passing to a function based on whether you pass them by pointer, or by reference/value. It also prevents a library from changing an argument from const T& to T&, introduce mutations, and break callers as a result.
> Exceptions
Google's code is just setup to std::terminate instead of throwing. It's a minor performance gain AFAIK but it also avoids littering the code with try {} catch {} blocks. It forces developers to handle errors instead of propagating them whenever possible too.
> “Use lambda expressions where appropriate.” – probably a cheap shot, but what is the alternative? – use them where it’s not appropriate?
This point seems counter-productive considering the author claims the guide is written in too much of a prohibitive fashion.
> Avoid complicated template programming.
TMP is hard, increases compilation times, and is difficult to reason about. The Guide recognizes its usefulness but suggests avoiding it where possible. This is similar to the library writer vs application writers argument: not everyone needs it, avoid it if possible.
All in all I think the author just doesn't have the same requirements, constraints, and sheer amount of developers/lines of code Google has. Nothing is forcing them to use the Guide. In fact, it's public but it was written for Google, by Google. It works for Google, and it's a great tool in that kind of organization.
Disclaimer: I work on Chrome, which has its own guide derived from the GSG.
Sounds to me like the author prefers the rule-based approach and/or is assuming these rules are strictly enforced.
It should be noted that any reasonably sized shop uses a subset of C++. It's insanity not to. It's just a question of what to allow and what not to.
Google C++ uses a style of error-handling that's akin to how Go handles errors. Many functions return a util::Status (which would be OK or an error) or, when you needed to return a value, return a util::StatusOr<T>.
Some people who write Go rail against the verbosity of this. I actually like it because it makes it pretty easy to reason about what your code is doing without the flow control you can get from exceptions.
There are definite warts in the codebase. One big one I remember was that one of the early developers (which might've been Craig Silverstein) didn't believe in unsigned types so there are a bunch of size functions that return an int and this has caused no end of problems.
I'm no C++ expert so I can't speak to some of the author's gripes (eg pragma-once vs #define/#ifndef) but a lot of the criticisms suggest that the author hasn't worked on large codebases.
Take operator overloading. This is fine, in theory, if used judiciously. The problem is engineers differ on what's reasonable and this may lead to unexpected behaviour, particularly when you start factoring in automatic type conversions. You see this in Scala, for example, where people go nuts, basically because they can and for no other reason.
There's a certain type of engineer who gets trapped in a mindset where they start considering complexity a virtue. I once got into a discussion with someone where he was shocked that I didn't know what perfect forwarding was. After looking it up I was like "why do I need to know this (unless I'm writing a templated library)?" and the answer is I don't.
Google's C++ style guide is largely about avoiding these flights of fancy making it into codebases that other people rely on, may need to debug, etc. And it achieves that goal as I found Google C++ code quite readable on the whole.
There's still a lot of code that doesn't, say, use smart pointers, so you can get dereferencing errors if you're not careful or if you don't understand who owns what. And this isn't something that C++11/14/17 smart pointers are perfect at anyway (something I'm quite bullish about for Rust, in comparison).
Another example: the complaint about no reference arguments. The point of this is so you can't tell what function arguments might be mutated if you allow non-const reference function arguments whereas with pointers it's obvious. To be clear:
foo(c); // could be pass by value or reference (const or non-const)
foo(&c); // clearly mutable
However this is vastly different than saying that we don't want any of these features anywhere because they might not be appropriate under some conditions. I advocate education so developers can make decisions when to use and not to use certain features (same as selecting the right algorithm -- we don't ban binary search b/c it's harder to understand than linear search).
Same with the "we have tons of programmers" argument -- let's use the lowest common denominator (everything else is "fancy"/"clever") to make sure every line of code is understandable by anyone. Encourage people to learn, not everyone else to dumb down.
-Wsign-compare
Warn when a comparison between signed and unsigned values could
produce an incorrect result when the signed value is converted to
unsigned. In C++, this warning is also enabled by -Wall. In C, it
is also enabled by -Wextra.
(And prior to that explicit option, it was just enabled as part of -Wextra/-W, from 1995: https://github.com/gcc-mirror/gcc/commit/7030c69607d547b3227... . For comparison, Google was founded in 1996.)
I buy that some of the integer promotion rules really suck, and Google's founders learned C prior to 1996. But the compiler has been able to warn us about this particular footgun for a long time.
Are `constexpr` values destructed when they go out of scope? Or is it simply not possible to `constexpr` them?
A constant is not a static variable.
Speaking for myself as an engineer who works in that codebase, I rarely find it gets in my way, and when I've felt exceptions were the clearest/best approach, I've not found it burdensome or difficult to justify them.
What? I cannot read more after this.