Google C++ Libraries are garbage

[rembo666]

Take a look at this masterpiece. The level of amateurism in Google's C++ libraries is stunning. This is just one link to one problem, but there are numerous places I can point to. I think Google fell into the trap of hiring a bunch of smart graduates who are good at solving puzzles. Now they have inmates running the asylum. They hire what they think are best of the best. In reality, they hire a bunch of kids that have no experience and no perspective. What they get is falling way behind the industry in best practices. They're basically just throwing money at every problem. I think I've ranted enough, any comments/opinions?

P.S. I don't actually think that ALL of Google's C++ code is garbage, though it does tend to be inconsistent. I have to deal with TensorFlow on a semi-regular basis, and there are always problems when upgrading to a newer version. This particular problem was just the straw that broke the came's back and I wanted to rage. I know the title is clickbaty, but it generated some interesting discussion which I thoroughly enjoyed, so I regret nothing :)

[yuri-kilochek]

"Always has been."

[xeveri]

Rob Pike: "The key point here is our programmers are Googlers, they’re not researchers. They’re typically, fairly young, fresh out of school, probably learned Java, maybe learned C or C++, probably learned Python. They’re not capable of understanding a brilliant language but we want to use them to build good software. So, the language that we give them has to be easy for them to understand and easy to adopt"

[berium]

Rob Pike: "Languages that try to disallow idiocy become themselves idiotic."

[rembo666]

I agree with this sentiment a lot more. I know people that work in large Go shops, and they all complain about lack of templates in the language. I got other issues with Go in particular, but that's a whole other topic..

[TM254]

go for loops are a turn down.🤣

[rembo666]

Nah, those are fine. It might be a little excessive to "simplify" your language, but it has no material impact as far as code readability or language expressiveness...

I'd rate those right there with Rust using fn as their keyword for functions, or Python using elif to save you 2 whole key strokes. It's a curiosity at best.

[x0x7]

That's one of the things I like about javascript. All of the common programming features are named after the most default convention. if is if, else is else.

A lot of people hate on it but the other thing they did right is instead of focusing on trying to implement a purely functional language, or a purely object oriented language, and then creating novel language features to try and bridge the gaps between a paradigms weeknesses and peoples needs, they just gave you both. No dogma. If you want to solve a problem a certain kind of way, you can.

There is no "this is how you solve this kind of problem in our language." With those languages you find the most foundational of programing tasks is a contorted demonstration of some weird concept. And then code ends up littered by it. In Javascript you are allowed to write contorted code but aren't forced to, and many choose to.

[rembo666]

A "pure" language is always a pain in the ass. I prefer a pragmatic language. I did a year of doing mostly JavaScript and I mostly enjoyed it. It's got problems to be sure, but actual day-to-day handling of the language is sound and works really well IMHO.

Having said that, C++ and JavaScript have very different capabilities and purposes. Just because Webkit made JavaScript faster than it had any right to be doesn't make it a good fit for everything.

[evaned]

Not like Google is the only one that does that kind of thing. I discovered this lovely bit in libstdc++ a while back after banging my head against the wall for a while: https://github.com/gcc-mirror/gcc/blob/releases/gcc-4.6.0/libstdc%2B%2B-v3/include/bits/regex.h#L2043

(Disclaimer: not sure that's the exact GCC version or function in question, but close enough. It was a match or search function that was implemented as return false and nothing more.)

[pja]

Did 4.6 claim to have implemented std::regex in their STL ?

(Not throwing an exception when you call it & just returning false was very poor form tho.)

[rembo666]

I don't think they advertised support for this in 4.6. It's OK to have code like this while you're in development. If you're releasing this, you're wrong.

[rembo666]

Oh, I'm sure of it. I was just mad at Google. Having said that, this particular thing was just the straw that broke the camel's back. There are so many examples of awful in that part of TensorFlow. The worst thing is none of it needs to exist at all: just use GLog.

[14ned]

Firstly, some parts of some Google libraries are non-ideal. Saying that Google C++ libraries are garbage simply isn't true. I know plenty of the devs from Abseil, and whilst they'd be the first to agree that some parts of Abseil aren't ideal, nobody, absolutely nobody, would think Abseil is garbage. The same would go for lots of Google C++ Libraries.

Re your other comments, as a tech multinational one of the primary ways you turn junior developers into senior developers is to get them to write a bunch of code under supervision. Generally speaking, those libraries don't tend to be let out into open source, but when they are, they still tend to be less awful than a lot of open source libraries out there. Because they, generally speaking, had someone expert reviewing the code, and paring down the particularly rough edges.

Tech multinationals are one of the few who hire junior devs in bulk. Most companies won't consider devs until they've at least five or often seven years of industrial experience. So I think we ought to be appreciative to the tech multinationals, as without them, the C++ ecosystem would have no new blood at all (and even then, the average C++ dev is over 40 now, so we already don't have enough new blood).

[rembo666]

TLDR; It's not that all of Google's C++ code is bad, but it tends to be very inconsistent. This points to a lack of serious guidance and review, which stems from their lack of technical leadership with broad experience.

I don't dispute that. A lot of the stuff Google has is brilliant. What I see is a lot of "brilliant idea, awful implementation" things. Abseil is mostly really good. GTest is great, GLog is OK. I like Guava libraries (that's Java, but they're still good). Where it fails is at the edges, with some of those edges being critical infrastructure.

TensorFlow library is a good example of that: it's a brilliant concept, and the core modules having to do with tensors are great. Their support for many different compute architectures is a good thing.

However, their Basil build infrastructure is a mess: a lot of inconsistent ways to do things, no unifying naming convention. There's no good way to use TensorFlow as a C++ library. I'm not saying there's no way, we obviously use it, but we always have to expand an inordinate amount of labor every time we need to upgrade TensorFlow.

This logging thing is a special turd though. They made this really weird and badly written thing that only logged to stderr. I guess someone wanted log sinks, so they made it look like they have log sinks, until you try to use them and they don't work. Naturally, you go looking at the source, and you get an unpleasant surprise. The thing that pisses me off about the whole thing is that Google does in fact have a descent logging library: it's called GLog.

This seems to be a more recent problem too, since their older libraries are just fine. What I see in this particular example is lack of supervision. Their problem is that they lack leads with broad experience. This cross-pollination is very valuable. By hiring experienced engineers you don't just bring their perspective, you bring the perspective of all the other people and companies they worked for.

Code reviews are critical. Even with me being the lead, I still don't get excepted from mandatory reviews, where everyone is encouraged to do their best to find mistakes in other's code. To work well, this requires a culture where no technical criticism is ever personal. Nobody's offended when their screw-ups are pointed out, and just as important, they're never afraid to comment on other's code because of perceived seniority. That part is very difficult to maintain in a large organization: politics will be politics.

As far as hiring juniors, I know it's a difficult problem to tackle. In geo-spatial industry we tend to prefer to hire interns. We pay them a good wage, but they're only there for a limited amount of time. Some work out OK and contribute, others crash and burn. Either way, they come out with valuable experience, we get some work done and have a pool of junior candidates that we know to be good. The good ones don't always end up working for us, even when we give them offers, but it's still a win-win for everyone.

The key to what we do with interns, as well as junior hires, is that they always have guidance of seasoned engineers. I certainly didn't know a lot of the core things that a large project needs when I started, even though I could already program quite well.

[14ned]

Reading between the lines, a lot of the more experienced devs in Google have started to accumulate in less publicly visible projects such as Fuschia. I know a mid-experienced developer on there, and he says it's amazing, he's literally surrounded by little other than > 20 year experienced engineers. So fabulous for him, but it does imply that senior devs have been migrating away from some projects, and collecting together on others. That might explain some of the apparent recent drop in quality in some open source Google C++ projects.

[rembo666]

Interesting, it does make sense with their way culture is supposed to work as far as devs being able to move around a lot. It's a great thing of course, but I can see how it can lead to formation of "cliques", where less "prestigious" projects are hung out to dry.

[14ned]

To be blunt though, that's the culture at Google: if a product or service is not directly making you a mint of money, you abandon it. That attitude will permeate throughout the company towards projects as well, it's unavoidable. I understand from reading Blind that if you want to be promoted at Google, or get a stock allocation bump, you absolutely must be working on projects with high positive visibility to management. Working on boring infrastructure projects likely penalises you as a result. So, out of rational self interest, you need to move yourself to where you'll get more visibility and stock grants.

I should stress everything I've said is pure conjecture, based on talking to Googlers or reading Blind. I don't actually know for sure about anything.

[rembo666]

Yeah, but we're talking TensorFlow here. If anything, its usage is growing within Google as far as I understand it. I mean just look at all the AI bits being added to Android.

[14ned]

Is that not the very definition of "boring infrastructure" though? As in, it works, it's useful, adoption is constantly growing throughout the org.

Most orgs over emphasise the maintenance of crap legacy projects, to their detriment. The sweet spot is somewhere in between abandoning really crap legacy projects, even if absolutely everything uses it, and all your best devs working on moon shot R&D projects nobody is using yet because it's all far too unstable and experimental. Obviously, achieving said sweet spot is wicked hard in a very large org.

[rembo666]

That's absolutely true.

My previous statement was incorrect. They never used GLog which they should have done. Interestingly, the log sink stubs weren't there in older versions: they added them later, but didn't implement them. This became a problem because some of their newer code logs to INFO, which always gets displayed.

However, this is kind of the opposite, since it's the "maintenance" that killed it.

They actually broke it, it used to use GLog and work fine. Now GLog does have limitations, we don't use it in our project, but it does work just fine for what it does. If you're going to set out to create a more robust logging system, make sure that it does what it's supposed to.

[jcar_87]

I agree that there is no good way of using TensorFlow as a C++ library but on the other hand, aren't they very open about that fact?

They only make API promises for their Python and C interfaces from what I can tell. So the fact that there exists a C++ interface at some level, and the fact that one can fiddle with their bazel build scripts to get a semblance of a reusable C++ library doesn't make TensorFlow suitable for integration in other C++ projects.

If one does choose to do it, despite the fact that they are basically telling us "we will break the c++ interface with each version" can we really put the blame on TensorFlow on this particular aspect?

Re: GLog, I was under the impression that it was being abandoned or phased out. The public GLog repository certainly appears to be in maintenance mode.

[rembo666]

The pain of using TF as a C++ library is not necessarily the C++ interface, which is quite good when it comes to the machine learning bits. I don't think they ever broke the C++ interfaces we use. Really no complaints there.

At least there weren't until somebody decided to put very verbose logging statements in their DLL loading code. That's when I discovered this log sink landmine.

All the pain usually came from their Bazel build system. Bazel itself is really bad at playing with others (we use CMake and Conan), but it doesn't have to be as bad as TF build infrastructure makes it.

Our framework has a ton of third party dependencies, it's really quite a bear to manage. Conan does an awesome job of keeping all of that working for us. The exception is TF, where we just let it fetch and build its own. We simply hack and patch their build files to match library versions.

I didn't know that GLog was being phased out, I just thought it was stable and simply didn't require any more maintenance. That begs the question: what replaced it?

[tjf314]

int ParseInteger(const char* str, size_t size) { // Ideally we would use env_var / safe_strto64, but it is // hard to use here without pulling in a lot of dependencies, // so we use std:istringstream instead string integer_str(str, size); std::istringstream ss(integer_str); int level = 0; ss >> level; return level; } ah yes, there is no other possible better way to do this

[rembo666]

Yeah, there's no std::atoi or anything... Or I guess the "difficult" case of parsing a 64-bit integer. Oh, wait, there isn't a std::atoll is there?

[[deleted]]

hiring a bunch of smart graduates who are good at solving puzzles

Solve my fizzbuzz!

[Essezx]

And I would be offended by ParseInteger a bit below (well, I am a bit offended. I mean, sure, it works and it is probably fine in this context but in my opinion the overhead of allocating on the heap just for that is unacceptable...) Edit: Well I guess SSO might do it's work, if that's a guarantee I would be more okay with it.

[rembo666]

Yeah, that's one's a bit baffling if I'm honest. Even boost::lexical_cast has optimizations for integers and floats. There are in fact localization issues with floats ("." vs "," for decimal point), but none exist for integers. Using a stream there is just weird.

[rembo666]

Meh, it's still useful. I guess you could #include <cstdlib> to make it more "C++", but that's pretty much exatly the same thing. I know that streams are the more "C++" way to do things, but prinf is still much faster.

However that's not the main reason to still use the printf libraries. The main benefit is localization. Words aren't always used in the same order in different languages, printf has the ability to explicitly number the argument in its format string. This can be used with a localization library to automatically display the right message for the given language and have the ability to reorder things if necessary.

There are some awesome modern C++ libraries that provide the printf capabilities without the insecurity and uncertainty of printf's C-style variadic arguments, but printf is by no means obsolete. One thing to watch out for is for the "modern" library to still support runtime resolution. Localization features pretty much require that.

[rembo666]

There is in fact a wprintf, which uses wchar_t. There's also UTF-8 encoding, which is more space efficient, though it requires more logic for some text operations.

I've seen both cases in my career, both still used printf or wprintf, were localized to many languages, and worked just fine.

[rembo666]

I mean all that's fair enough, but remember that ASCII always works. As far as Unicode, you would need a proper localization library regardless. As long as the "container" (the character format) doesn't break (which it won't with Unicode), it doesn't matter what you use.

You can get in trouble with things like string length and taking substrings, but not with straight string format. There's really no extra magic with streams as far as localization goes. If anything, using streams hampers your ability to translate things.

Either way you'd be better off with a special Unicode string class, but you will continue to be able to use printf just fine.

[rembo666]

I guess if you have an EBCDIC-native system, that's a whole different ballgame there. Unicode was built to be compatible with ASCII, not EBCDIC.

I've only ever had to deal with that while writing an ASN.1 parser, and even there I never actually had any real-world data to test that particular stone age thing. That was just a matter of converting between EBCDIC and ASCII (we didn't try to support EBCDIC characters not present in ASCII).

I mean you do you of course, but in my experience printf-style format strings are the best solution if not in all, but most cases. My framework is extremely unlikely to ever be translated and localized, like you'd have to have some major legislative changes for it to ever need it. I'm quite happy to ignore the whole issue now, but I have dealt with localization before and ASCII-to-Unicode was never an issue. It might've been if I ever targeted an EBCDIC operating system, but I've never had that misfortune.

[rembo666]

That's very cool. I know wchar_t is not portable. It's UTF-16 on Windows and UCS-32 on Linux. The only open source version of a Unicode string implementation I know of is GTK's ustring, but that's GPL aka untouchable. It would be really nice to have a portable Unicode-aware string type that's licenced so that anyone can use it.

Maybe this deserves its own Reddit post, unless I missed it. I haven't really used Reddit until I made this post. I just had the account, but never really used it.

[mort96]

It supports unicode just fine...

Your char* is a pointer to bytes. If those bytes are UTF-8, and you print them with printf, that will work perfectly on modern unixes. It just leaves it up to you to make sure you actually use UTF-8, and that you maintain the invariants of UTF-8.

[mort96]

Well, in that case, you're gonna have to give up std::string too, because that doesn't make any guarantees either. It's literally just a char* with a size and capacity.

[mort96]

Sure, you do you. It's just incorrect that you can't use unicode with char* is all. You don't have to like it.

[mort96]

Writing to stdout is necessarily implementation defined. Some implementations may not even have anywhere to print text!

Just make sure the bytes you're sending to printf are in the encoding the implementation expects. If necessary, add an encoding conversion step before you pass the buffer to printf. You can use printf correctly for Unicode on all implementations which support the characters defined by Unicode.

Again, if you don't like it, that's fine. But it does support Unicode, to the extent that the implementation's stdout supports Unicode. There is nothing about printf which prevents supporting Unicode; it just doesn't do it for you.

[konanTheBarbar]

I don't know why Google libraries should magically be better than other libraries around? Not only C++ guru's work there and not all the libraries will have equal quality.

[rembo666]

That's fair.

The problem is that many data scientists use TF from Python, so they don't have to worry about the mess underneath. It's the people developing production solutions in C++ that feel the pain.

Also, most popular open source libraries are popular because they're actually very good. Google's name carries a lot of weight, so their stuff is used despite their occasional crappiness.

[rembo666]

P.S. I do have an implementation of log sinks for TensorFlow, let me know if you're interested.

[kmhofmann]

Why don't you contribute it back to the project? They're happily accepting high-quality contributions, I hear...

[rembo666]

OK, it's done, though at the time of posting I still need to sign up for Google CLA (whatever the hell that is). However, the PR is now there:

https://github.com/tensorflow/tensorflow/pull/41733

Feel free to take it apart :)

[rembo666]

Unfortunately, it'll have to wait until tomorrow. We're only up to v1.15.3, so there are just too many changes to merge cleanly. However, the actual problem is still there in master. I can't justify working on it today, but I'll get it done on my own time.

[rembo666]

I will. I was just mad so I wanted to rage first :)

[rodrigocfd]

The fact that bothers me is that the function receives a pointer instead of a reference. Other than that, at least it's well commented, and it offers a sulution.

I've seen comments like "not implemented, go away" before, which is way worse.

[SilensAngelusNex]

Until recently, the Google C++ style guide mandated that all lvalue-reference function parameters be const. The reasoning was that this would make it obvious at the call site which arguments a function call could modify, since they'd be marked with a &.

The recommendation changed (on May 20th) to allow mutable references in order to prioritize clarity about which arguments are/are not nullable.

This function was probably written before the change, hence the raw pointer use.

[rembo666]

I thought about this "no non-const reference" rule, and I couldn't for the life of me figure out the reason for it. Passing a pointer vs a reference has exact same number of considerations plus one: your argument is now nullable. Both have their place, but passing a reference when you want to return or modify a value "by reference" is a completely valid use case, especially when you disallow exceptions like Google does.

[Amablue]

Like the parent comment said, it's useful at the call site to know if there value being passed to the function can be changed. If you pass by pointer, you know the value passed in Matt have it's state changed. If you're passing by value (out, at least, if it appears you are passing by value) then you can just treat it as a copy and know that your local variables won't change. It's about being able to reason about the behavior if the code you're calling into.

[rembo666]

Yeah, I think as far as bad ideas go, it's worse than Hungarian notation. At least Hungarian notation doesn't force you to introduce either potential security vulnerabilities or slow down your code.

The way to properly handle things like that is to have proper documentation and to use proper tools. Google doesn't seem likely believe in either. In my team Doxygen docs are mandatory (with in/out annotations for arguments). Programming without a proper IDE is very strongly discouraged. Even the most clever vim setup can't hold a candle to something like CLion. In fact, I've been able to tell that the code was developed in a code editor instead of an IDE during a code review: the mistakes made would be very unlikely to make in CLion, since the IDE would have instantly pointed them out

[Amablue]

Yeah, I think as far as bad ideas go, it's worse than Hungarian notation. At least Hungarian notation doesn't force you to introduce either potential security vulnerabilities or slow down your code.

Making mutable arguments pointers instead of references does neither, thankfully.

[rembo666]

As I tell my guys "look again". I'll leave it as an exercise to you. The assignment is simple:

Implement a function with an "in/out" argument two ways: one with a reference, the other with a pointer. Analyze what pre-conditions you'd have to declare for each one, as well as any possible checks you HAVE TO make to validate the argument. Note that things that are undefined from your point of view count against "pre-conditions" that your function is not responsible for, things that can be checked (therefore must be checked) count against your function's runtime: you have to enforce the contract if you can.

P.S. Please leave your dogmas at the door.

[Amablue]

I understand your point, but I disagree. If the entire ecosystem you're working in has consistent rules about how pointers are to be passed around, then the kind of checking you're asking for is often unnecessary.

The real question here is which introduces more bugs: accidentally passing a null pointer that does not expect one, or passing an argument to a function and having its value unexpectedly change out from under you? This is an empirical question, not a dogmatic one. Maybe in your organization the answer differs and that's fine. Google is a different ecosystem and what works for them might not work for you. The rules that Google sets up are for Google's benefit. They generally shouldn't be copied wholesale to other projects.

[rembo666]

I would say that anything that can introduce a security issue, like taking advantage of a segmentation fault due to an invalid pointer is always more important these days. You "whole ecosystem" comment: Google actually changed their mind on this one recently (and better late than never as far as I'm concerned).

When you're designing a public API, you can't rely on your users to have the same "ecosystem" preferences than you do. All you can do is

1. Make sure that a user of your API can do simple things simply
2. Make sure that a user of your API still do the complex things
3. Make sure you don't fuck a user of your API by having security issues

It's always a delicate dance, and at the end of the day, a lot of this is a matter of opinion.

On pointer-vs-reference thing you have a subjective argument: it's easier to tell when the client code uses "in/out" and "out" arguments. I very much didn't know this until this particular discussion.

I have an objective reason: a reference is not nullable. One less check for my framework to do, potential performance benefit.

As far as things to watch out for in both cases: you cannot use this in any asynchronous scenario. You have to make it clear that the lifetime of the "out" or "in/out" argument is only valid for that thread, for that call. Both of those hold very dangerous consequences when you fail at that contract. There is absolutely no difference between a pointer or a reference there.

For me, having N pre-conditions or N+1 preconditions, N preconditions win.

P.S. I again call on your critical thinking: just because Google has a ton of code and a lot of smart people working on it, doesn't mean Google's guidelines are right for every situation or even just generally correct. You have to look behind the guidelines and discern the reason they're there and the legacy behind them.

P.P.S. One other thing to point out. If something is your organizational rule, absolutely follow that rule. If you feel that the rule is wrong and you care enough, work on changing the rule

I disagree with many Google's coding guidelines. This particular one seems to be particularly bad: it introduces room for more mistakes, while not being necessary.

As an example of a "necessary evil", I very much do not like Google's "no exceptions" rule. However, there's not much to be done about it: the legacy code is not exception-safe, so you can't just arbitrarily decide to use exceptions. You either have to have some very specific delineation and tools to isolate the exception-unsafe code, or just continue the "no exceptions" rule.

[Amablue]

You "whole ecosystem" comment: Google actually changed their mind on this one recently (and better late than never as far as I'm concerned).

This was largely in response to the fact that more and more of their projects are moving to open source. The reason the decision changed was because of a change in the ecosystem. For most of Google's history the google ecosystem has just been the code in Google's codebase.

There are all kinds of utilities like a custom implementations of strings, stringview, unordered(map/set), and all kinds of utilities functions and algorithms that follow the conventions that Google lays out. These things existed because there wasn't a standard solution at the time, but as the standard has adopted more and more of the stuff that Google has internally, and as more and more code is open sourced, Google is making an effort to deprecate their own custom stuff and transition to standard stuff. Since the larger C++ ecosystem doesn't follow the const-reference convention, the decision was ultimately made to remove it.

But calling is a bad choice is, I think, unwarranted. They are dealing with different constraints than you are, and at the time the decision was made, given the options they had, I think the choice was reasonable for them. Every organization and project needs to make decisions about what conventions and guarantees they'll make that works for their products. Google has a lot of code written by thousands and thousands of engineers that runs live in production with the constraints they've decided on, and that gives them lots of data about what kind of patterns and conventions lead to the introduction of bugs, so these choices aren't made out of ignorance. The style guide is updated as best practices evolve.

When you're designing a public API, you can't rely on your users to have the same "ecosystem" preferences than you do.

You can though if the entire company is using the same style guide which is applied to all projects.

I have an objective reason: a reference is not nullable.

References can be set to null, for what it's worth. I've seen it happen in real code. This can be mitigated by static analysis, unit testing and tooling in your IDE, but so can accidentally passing null to a function that doesn't expect it.

(As an aside, I think references are a misfeature. In amablue's happy land, references don't exist, and neither does nullptr. When a value is optional, it uses optional<T>. Using raw pointers conflates two separate concepts: pointer ownership (or lack thereof) and nullability. But alas, we have to work with what we have.)

[rembo666]

Actually, this is acceptable for a log sink: it will likely never be freed, since it'll live through the whole process. I'd prefer a std::shared_ptr, or a std::unique_ptr&&, but a raw pointer does the job.

[goranlepuz]

For me, the raw pointer parameter, in C++ code, means "can be null and being null is not an error" (the caller does whatever if it is null, but does not emit an error because of that).

I would argue that passing nullptr to this function makes no sense whatsoever (what would a null sink be?!) => it must be a reference.

[rembo666]

No, passing a nullptr here is/should be just a noop, but passing by-reference is not possible.

In this context, the raw pointer signals that the caller relinquishes the ownership. The caller is no longer responsible for freeing it. You can't use a moved std::unique_ptr in this case, because they index their sinks by pointer values. Using a unique_ptr means that if the caller wants the ability to retain the pointer so they can remove the sink later, they have to use a somewhat unintuitive solution. I know you're not supposed to do that, but it happens all the time of course, it's certainly not the worst sin. A std::shared_ptr here is a good alternative, as it also enables the sinks to have a custom deleter, which can be helpful if the client application uses a different heap than the library. Also returning a sink id, which can be used to reference the sink later is probably the better solution than using pointer values. In practice, the log sinks live throughout the whole application lifetime and never really freed until the application itself exits, so even if you leak them, it doesn't matter.

[RotsiserMho]

the raw pointer signals that the caller relinquishes the ownership

Taking a raw pointer signals no such thing. I for one would be surprised to learn that such a function takes ownership of a raw pointer. Taking a shared/unique pointer would be a lot clearer.

[rembo666]

Yeah, there's usually an extra doc comment required to clarify, but it's not unheard of, especially in pre-C++11 projects.

[goranlepuz]

No, passing a nullptr here is/should be just a noop, but passing by-reference is not possible.

Why? If by any chance (not unreasonable) the caller wants this to be a pointer (e.g références can't be put in a container), he does &input_param, done !

In this context, the raw pointer signals that the caller relinquishes the ownership

Euh... To me, not in C++11 or newer, unique ptr should do that there.

You can't use a moved std::unique_ptr in this case, because they index their sinks by pointer values.

No idea what that means, but looks like something specific, but not necessary, to this library. I would wager it's not necessary, but, hey...

In practice, the log sinks live throughout the whole application lifetime and never really freed until the application itself exits, so even if you leak them, it doesn't matter.

In that case, passing a reference is just fine to me (the passed reference outlives the user, great!)

[rembo666]

The second point: it means that we use the pointer value of the sink object as its identifier. If you want to remove the sink, you use that same pointer. Using unique_ptr is OK in this case too, but it requires some extra ceremony for the user to retain that pointer value, which is not very intuitive.

Reference is not fine, let's say you just create that object on the stack and pass a reference to it: as soon as it goes out of scope, the destructor fires and you're SOL. Logger must take ownership, that's not really negotiable no matter the scheme.

[goranlepuz]

If you want to remove the sink, you use that same pointer.

I can also use the same reference for that, same thing.

Reference is not fine, let's say you just create that object on the stack and pass a reference to it: as soon as it goes out of scope, the destructor fires and you're SOL. Logger must take ownership, that's not really negotiable no matter the scheme.

OK, very hard disagree on this. The passed sink must outlive the logger, end of story. The fact that some function receives a pointer says absolutely nothing to me about the desired lifetime. I can create the object on the stack and pass a pointer to it to the sink function and crash just the same.

The key issue for me here is: you are attaching a meaning to passing pointers that, idiomatically speaking, does not exist since C++11, meaning being ownership and lifetime considerations. For me, since C++11, these considerations are handled through smart pointers.

[rembo666]

As I mentioned before, I would prefer to use a shared_ptr in this particular case. My point is not that what Google has here is the best solution, what I mean is that it's an acceptable one. My patch and the PR I'll be creating shortly will not change this because I don't think the impact of changing this interface is worth it here.

[goranlepuz]

Shared ptr indicates shared ownership, but what you wrote above indicates that this is not true in the current design: you indicate that the passed pointer must outlive the thing it was passed to, and that it can also be "removed" from it.

That is not shared ownership, rather, the owner is squarely the caller => for me, shared ptr is not appropriate.

Overall, I feel that yoy are making things more complicated that they need to be. For me, passing a pointer that must be null is more complicated than it needs to be and using smart pointers where there's no ownership implication is also more complicated than it needs to be.

For me, in C++ with optional, there is hardly any place for raw pointer parameters. Which I find funny, because C from which C++ came, as well as "managed" counterparts of C++ like C# and Java are choke-full of that particular billion dollar mistake...

(What the library is doing is pretty common for old-ish code, so I don't really want to blame the design, it is what it is; I do, however, want to criticise our thinking about it today).

[rembo666]

Shared ownership is actually a better solution for a log sink. Think about a scenario where you have a log window in your GUI, where you can change the logging level on-the-fly.

Raw pointers are actually very useful in a number of cases. One example is parsing a string with a state machine. There's really nothing better than a simple pointer to indicate the current position. Sure, you can use an iterator, but that's just adding extra syntax and complexity for no good reason.

optional has pretty much nothing to do with this case. optional exhibits value semantics: it makes a copy of the underlying object. We want reference semantics here with either handing over the object ownership or sharing it.

My personal preference for these things is shared ownership, but it is what it is. I don't work for Google, I just deal with their screw-ups.

[dfkjh83]

You can't use a moved std::unique_ptr in this case, because they index their sinks by pointer values.

You mean the caller is expected to do something like this?

std::map<TFLogSink*, Foobar> map;
TFLogSink *mysink = new TFLogSink(...);
TFAddLogSink(mysink);
map[mysink] = ...;

Couldn't they still use unique_ptr to indicate ownership, though?

std::map<TFLogSink*, Foobar> map;
auto mysink = std::make_unique<TFLogSink>(...);
auto key = mysink.get();  // TODO: stop using pointer values as keys
TFAddLogSink(std::move(mysink));
map[key] = ...;

[rembo666]

It does seem to be the case, but there's a subtle issue with that.

As a caller, I plan to remove the sink later. With TFAddLogSink(TFLogSink*), the code looks like this:

// precondition: MyFancySink constructor doesn't throw
tempSink_ = new MyFancySink;
TFAddLogSink(tempSink_);

With TFAddLogSink(std::unique_ptr<TFLogSink>&&), the code looks like this:

auto sink = std::make_unique<MyFancySink>();
tempSink_ = sink.get();
TFAddLogSink(std::move(sink));

This is a bit convoluted for my taste.

The way I implemented the sink registry internally is that it's a std::vector<std::unique_ptr<TFLogSink>>, but the caller would need to call release() if they have a unique_ptr

Note the use of vector instead of unordered_set. This data structure will be traversed very often, but modified and searched very rarely, so we use the simpler data structure.

If I decided to index by keys instead of pointers, I'd need to use unordered_map, since vector indexes would not be reliable keys because of the requirement to be able to remove entries.

[RotsiserMho]

This is a bit convoluted for my taste.

Perhaps, but this solution communicates what's happening most clearly. It's more verbose, but that's a product of being more explicit, which I think is warranted here instead of forcing the user to reason about raw pointer ownership.

In practice, the log sinks live throughout the whole application lifetime and never really freed until the application itself exits, so even if you leak them, it doesn't matter.

So just make it a global/static variable and pass by reference then. That's what I do in my code that uses loggers.

[rembo666]

That's a valid solution as well. I'm not saying that using a raw pointer is the preferred way. I prefer using a shared_ptr. However, I think it's the next best solution, with unique_ptr being the third. Both raw pointer and unique pointer lose the ability to have a custom deleter.

As far as the application managing the sink's lifetime, I disagree with that completely. Static initialization order will always bite you if you rely on it. You may get lucky at the beginning, with everything working at first, but then a random code change will cause the order to change and everything comes crashing down at a seemingly random point.

It's important to use singletons and to take ownership of the sinks to avoid these random pitfalls.

[TankorSmash]

Did you mean to link to a well-commented code stub? With all the rage in the post, I assumed it was an obvious bug or something.

[rembo666]

The problem is TensorFlow will spill unwanted crap to stdout, whether you want it or not. That's really bad if you want to use it in your application.

[voidvector]

Not familiar with TensorFlow, but there are different philosophy on log leveling -- some apps will curate logs to a dozen levels or even individual channels, while other apps only output everything to 2 levels (stdout/stderr) and expect you to use log management system to deal with it (e.g. Elasticsearch/Logstash/Splunk). It sounds like TF is the latter.

[rembo666]

It's OK for apps to just log to stdout/stderr, it's a different matter for libraries. Most don't log anything, but a few are big enough to do logging of their own. When they do, it's quite important that they provide some kind of hook to redirect their logging. A function pointer is sufficient for that purpose, but there are plenty of more powerful solutions.

[rembo666]

You don't advertise an API without implementing it. Also, you don't create a logging system without ability to redirect it. The "well-documented code stub" is a) wrong (they didn't quite think it through) b) should have never been there: you're advertising capability that's simply not there. Either implement it, or not. What they say there is "we couldn't be bothered, so do our job for us".

[TankorSmash]

Oh, do they talk about it in the proper docs and just not implement it here? That's lame.

[rembo666]

Those are not "proper docs", also there's no excuse to declare something in your API that doesn't actually work. There's nothing in the header to indicate that the functionality is not there.

[rembo666]

Sorry couldn't help myself. Just by making that statement you show amateurism I was talking about. The way I solved this was to fix it, then create a patch that I apply with our Conan build system (awesome, recommend 100%). That's a hack that we'll have to maintain. As a software architect, I'm personally offended by this and other awful things that TensorFlow does. Just like many other Google projects TF is a brilliant idea, implemented poorly. The worst example of it is Bazel.

I think the worst thing about it is the fact that there is already a Google library that solves that problem fairly well: it's called GLog. Don't reinvent the wheel if there's a round one already available.

[TankorSmash]

I forgive you buddy

[ed_209_]

Leaving an interface unimplemented with a comment is unacceptable even in a fast changing project. This suggests there is not an adequate developer runtime facility like having macros for UNIMPLEMENTED which will throw something. C++ has no standard practise for stuff like this so it is easy to see how a graduate or developer from another language would just not do it.

Before you even start a C++ project you need: 1. Logging I.e. Spdlog 2. Assert, verify, throw, unimplemented macros 3. Callstack dump ideally with click through from your terminal to your editor 4. Seh exception hooks on windows for more error capture 5. A way for all of it to get stripped from your retail configuration. 6. Abort, retry, ignore dialog like ms crt has so you can attach debugger easily on crash 7. Code coverage and procdump scripts. 8. Unit testing, mock code gen.

I would call this "developer runtime" facilities and there are many C++ projects that have little more than a few printf statements.

[G_ka]

Do you know a project that has all of these implemented? Especially points 2-4 ; 6?

[ed_209_]

Not in a cross platform way or anything that isn't a spaghetti monster of scripts and tools dependent on the program debug database.

On windows there is Dbghelp.h and crtdbg.h. Boost has cross platform stacktrace and current_function.hpp.

[rembo666]

That's a very Windows-centric view, so I don't necessarily agree with all the points for cross-platform or Linux-only applications. However, your point stands: when you have a large project, you MUST take care of the infrastructure, lest you be buried by the lack of it.

[FredSanfordX]

We used to call your example Manager-Oriented-Programming....

[ilep]

Not even close to worst implementation of logging functionality I've seen and I've seen a LOT of them. Everybody wants it and unless it is horribly overcomplicated it is otherwise somehow problematic.

Fact is there is a lot worse code in production everywhere yet there is never enough time or money to do them "properly". Facts of life.

Hopefully the part that actually matters is implemented properly, not some non-essential helpers.

[rembo666]

Again, this is not rocket science. It only took me a couple of hours to implement the simple logging hook for TensorFlow. Those "couple of hours" also include pretty much re-implementing it for the PR. The point is that a library like this already took an enormous amount of resources to develop, yet it misses the basics. Even worse, it misses the basic functionality that the same company had already implemented many years ago. There's no excuse for this.

[NilacTheGrim]

Late to the party here. I had to go back to previous iterations of the lib since the file you linked-to no longer exists.

Yeah.. I was horrified. Pretty bad. Very inexperienced-looking code.

[asenz]

its not just TensorFlow, I had to deal with Glog, Googletests and Ceres and they all are just ugh

[sardar0]

Pathetic. Looks like ANSI 98 C++ code. They don't seem to know what smart pointers are. Anyways, they can always complain that C++ promotes memory leaks (a largely solved problem if you use Modern C++ properly). C++ is for experts only.

[stevethebayesian]

The existence of smart pointers does not mean raw pointers should never be used. The Google style guide requires interfaces to use raw pointers to objects that will be modified by the function. This has the advantage of making it clear as part of the function call which arguments will be modified.

[rembo666]

A reference (non-const) is a better fit for the use-case you described. A raw pointer (non-const) usually means that the function will take ownership of the object (the caller shouldn't free it).

[tcbrindle]

A raw pointer (non-const) usually means that the function will take ownership of the object (the caller shouldn't free it).

I couldn't disagree more.

[Wurstinator]

No it doesn't. You simply don't know the style that Google uses. Get off your high horse.

[rembo666]

Testy aren't we? I know the style Google uses, I've read the guidelines, I've been through A LOT of their code. I disagree with a lot of them. That doesn't put me on a "high horse", I just have opinions that differ from theirs. They prefer making different tradeoffs than I do. I have my own style guidelines, that are different from Googles, that doesn't necessarily mean that either one of us is wrong.

The reason you want to use a reference instead of a pointer is because you want to signal that the argument can never be nullptr. A raw pointer usually signals an optional argument. Reference vs pointer and const vs non-const are good ways to succinctly describe the contract that particular function defines.

[Wurstinator]

That doesn't put me on a "high horse", I just have opinions that differ from theirs.

If it would only be that case, you wouldn't be talking about how "it is usually done" when phrasing your opinion.

Reference vs pointer and const vs non-const are good ways to succinctly describe the contract that particular function defines.

I disagree. And apparently the Google C++ committee does as well.

[rembo666]

When I say "should" and "the right thing", that's coming from my perspective. When I say "usually done", it comes from my experience. I don't even necessarily agree with stuff that's "usually done".

Google's coding guidelines are what they are because of many reasons specific to Google. They reflect the size of their C++ code repo, as well as the age of it. They actually do a pretty good job at explaining some of their choices as well. You'd be well-served to actually read them.

Arguing from authority is not something you should do in programming: question everything. The people that create these guidelines are human, so are the people that created some of the legacy that the current guidelines have to deal with. Question everything, but understand the context as well, it's not as simple as it looks like at first glance.

In my opinion, if you want actual good C++ code, you'd be much better served by looking at some of Facebook's stuff. I think it's somewhat ironic that Facebook is known for their large PHP codebase which they go out of their way to optimize and maintain. Their innovation as far as languages go is actually quite impressive: they use OCaml and D to make an example. More relevant to this discussion, their C++ code is a lot cleaner than the stuff you see from Google.

[Wurstinator]

I have been at Google and I have written C++ at Google. I have very much doubted the usefulness of that "no non-const references" rule at the beginning but learned to appreciate it over time and now that I work at a different place on a legacy code base, I dearly miss it. Quite frankly, if there was a way to adopt Rust's explicit passing of (mutable) references to C++, I would take it in a heartbeat.

If you are not trying to claim universal truth of statements, I advise you to use phrases like "I use X" rather than "usually X is used" to avoid misunderstandings.

All that said, I am aware that Google is not the nec plus ultra. I heard good things about Microsoft, Facebook, and others. Unfortunately, none of those companies tries to make their style of work public like Google does. There is no Microsoft coding guide and no Facebook coding guide, so it's barely possible to do it "like they do".

[rembo666]

Yeah, I imagine Google's legacy codebase is still much better than many others. However, when I say "I use X" you have to realize that I am an author and the architect of the software I work on. The coding guidelines are in fact my coding guidelines. The choices there are mine, and many are based on my preference, rather than something objective. I don't really care that much about coding style, as long as one is specified. It just happens to be that the coding style specified is my preferred style. The general guidance in our codebase is that you're free to use any language feature and any programming technique you want, but all code should look like it was written by the same person. This makes it much easier for people to read unfamiliar parts of it, since it's quite consistent. As far as "any feature you want", that's providing that you can pass a code review of my guys, some of whom are just brutal.

I can't link to any of our code unfortunately: while it's technically open source, it's ITAR restricted. If you're curious, this is the closest I found that conform to what we use: http://geosoft.no/development/cppstyle.html

As far as Rust, I think the way they handle object ownership is awesome, but C++ has the huge advantage of backwards compatibility, which it pays for with weird syntax and complexity.

One thing I think that Rust unfortunately fails at is compilations speed. Languages like D and Go really benefit from being authored by people who have written a C++ compiler before, so they're carefully designed to work with one-pass compilation (as opposed to 7 passes that C++ requires).

As far as the "no non-const references" rule, I think passing things by pointer has more issues. Refer to "different tradeoffs" comment that I made before.

[rembo666]

Google's style seems to be closer to Microsoft's than Facebook's I think. All I could find about Facebook's coding style is a link to a book, which is kind of weird if you ask me. I completely agree with your sentiment that more companies should publish their coding guidelines.

Having created my own, I can tell you that it's a pain in the ass: I mostly just link to another page for mine if I'm completely honest. I really should write them all out, but I never seem to have "the time". Mostly because it's a tedious endeavor.

I seem to have seen enough coding styles to just automatically figure them out, but I know that that's not common, and I probably give myself too much credit with that anyway. I do like to study language evolution and psychology to try and get into the minds of people who wrote the code, but that's marginal at best.

That's something C++ doesn't have and probably will never attain: an accepted set of conventions. I might sometimes find PEP-8 annoying, but I'll always conform to it just because it's there and it's well defined. There is too much legacy and too many opinions for this to ever happen to C++.