CppCon 2017: Louis Brandy “Curiously Recurring C++ Bugs at Facebook”

CppCon.org
-
Presentation Slides, PDFs, Source Code and other presenter materials are available at: github.com/Cpp...
-
I've spent the last few years watching Facebook's C++ codebase grow by several orders of magnitude. Despite constantly improving abstractions, constantly improving tooling, frequent internal courses, and ongoing internal discussion, there are bug-patterns we simply cannot stop from being reintroduced into our code. My hope is to show some of the most common (and infamous) bugs in our history, and the surprising complexity that arises in some apparently simple situations.
This talk serves the dual purpose of educating the intermediate (and perhaps the occasional advanced) C++ programmer about some really nasty common pitfalls, as well as serves as a plea to experts to help further improve the language, libraries, and best practices to help educate and eradicate some of these problematic patterns.
-
Louis Brandy: Engineering Director, Facebook
My team is responsible for the overall health of the Facebook C++ codebase, both the tools and the libraries. We work on: compilers, static/dynamic analysis, linters, large scale changes, and the core libraries.
-
Videos Filmed & Edited by Bash Films: www.BashFilms.com
*-----*
Register Now For CppCon 2022: cppcon.org/reg...
*-----*

[@NicholasMati]

I've been working as an embedded firmware / controls engineer in the Bay Area for over 6 year. It's fascinating to see the differences in dialect that develop with differing use cases. For example, I am rather familiar with the volatile keyword - its pitfalls, best practices, optimization impacts, and limitations in preventing race conditions. It, more or less, inevitably shows up when you go to write a driver for a microcontroller peripheral. I could easily give a half-hour talk on volatile. At the same time, today is the first day I have ever seen std::map.

[@jeremyed9507]

Wow, I knew all about the map cases but forgot everything I learned about volatile because I just don't use it.

[@Salzui]

And I would suggest you forget about std::map immediately again. At most use unordered map

[@ContemptuousCornbread]

Great talk. The guy is charismatic, gets to the point and has great pacing.

[@abcq2]

"you can't just say 'mutex!' and expect something to happen." "I didn't say it, I *declared* it."

[@jimcameron6803]

Stopped the video when he suggested searching for unnamed mutex locks. Searched our current codebase for unnamed mutex locks. Found an unnamed mutex lock. ... OK, you win this one.

[@thattechguy1398]

Two years later, I spent a week debugging this issue and I saw this video. You called it

[@comradepeter87]

The more I watch these sorts of videos the more glad I am learning C++ after Rust XD (for expanding job opportunities, not that I wanted to switch ;) )

[@BlairdBlaird]

It's really interesting how safe rust pretty much fixes it all (sometimes at the cost of convenience but still...), you will need to spend time understanding and satisfying the compiler but none of these issues has to be resident in the back of you brain, the compiler will tell you to take a hike bug 1: indexing is checked, period, though "range-based indexing" is also very much the standard bug 2: map doesn't have indexing, and reports lookup failure, and will never insert under you, it's interesting that the language committee itself has taken the "ban it" approach (but the API is also much richer) bug 3: lifetimes prevent this issue bug 4: volatile (both read and write) are unsafe operations on raw pointers; atomics are just... right there (though `AtomicPtr` also works on raw pointers) bug 5: the API of Arc (shared_ptr) doesn't allow this, it will only hand out a mutable reference to the T if you've got the only outstanding copy bug 5 bonus: same as (3), the lifetime of the reference is tied to the Arc instance, if the Arc is dead the reference is not valid bug 6: these are largely weird peculiarities of C++ syntax, rust syntax has its peculiarities (e.g. referencing in v out of a block) but jettisoning compatibility with C reduces the corner cases a lot, there really is no confusion between declaration and initialisation, furthermore: 6.1 the designers of the mutex API had the really smart idea to make the data protected by the mutex *owned by the mutex*, so the lock guard is a smart pointer to the protected data, therefore for "data mutexes" it's really hard to make this mistake. You can make it for non-data mutexes though, `MutexGuard` is tagged "must_use" (similar to C++'s nodiscard) which helps a bit but does not always save your bacon (this also applies to RAII types in general, if you create one you should probably tag it as must_use); Rust also has a trap here: `let _ = foo();` and `let _foo = foo();` have completely different behaviour, the second one will keep an RAII type alive to the end of scope but the first one will not: play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=34ffee864ed8f93f6e81c7741cd96ebc 6.3 the code is not confusing in rust, but it *does* allow same-scope shadowing, so you can do that. It just looks exactly like what it is.

[@personator]

Learning some rust was so beneficial to my ability to write C++ code. When you are forced to think about value lifetime, it makes you a lot more aware of it in other languages, especially if your previous experience was in a managed language where ownership doesn't matter.

[@tongpoo8985]

@@personator Yeah man same. I think of it like Rust is training wheels for C++, but the training wheels don't slow your cruising speed much, and if the training wheels ever touch the ground, you get a little bonk on the head. With C++ you don't have the training wheels and don't find out you messed up until you've crashed into a tree.

[@iandrsaurri625]

7:50 I'm a new C++ dev and I must admit, i was shocked seeing that map[] problem. I had no clue it would just make a new entry in the map

[@asdfasdfasdfasdfadsf]

"and then think a little bit more existentially, like what are we doing, and why are we here? I feel like I'm wasting my life a little....ok!" @30:43 lol!

[@TheOnlyAndreySotnikov]

If you follow Herb's guidelines and routinely declare local variables as "auto g = unique_lock(m_mutex)", the problem disappears. Or, actually, you can just right "auto g = unique_lock{m_mutex}". The template parameter will be inferred.

[@Ryanfriedman96]

Reference?

[@rutabega306]

I like this

[@sephirostoy]

Constructing an object using: Type(var); to declare a variable "var" of type "Type" should just never existed. No one use it anymore except bug makers... This is confusing for everyone: developers, compilers (make them more complex), any other parsing tools. Hope future C++ standards will remove it the same way as trigraph.

[@jakearkinstall5313]

sephirostoy I'm inclined to agree. However, I wouldn't be surprised if someone comes along and provides an example (that I haven't even considered) of where it is very useful.

[@DavidFreeseLee]

I would love to see an example of that, because I have never seen that before. Those examples just felt dirty.

[@michal.gawron]

Of course it's useful. It's useful when you want to troll people. gist.github.com/aras-p/6224951

[@Tupster]

It exists because you'd have to specifically detect it and exclude it because it just happens to be part of a broader set of valid but much less confusing parses.

[@N....]

How do you plan to make pointers and references to functions then? Or do you want to special-case the syntax?

[@brunomarques309]

Such an entertaining talk :D Thank you!

[@CppCon]

Glad you enjoyed it!

[@allopeth]

Great Talk! Very charismatic indeed

[@aryangupta9034]

That last bug with the mutex and shadowed lock (31:55). I had that before. Tore my hair out for a few days trying to figure out why my code wouldn't work.

[@ArjanvanVught]

“volatile” -> basically for interrupt coding only

[@JustSome16]

Also mapped memory.

[@digitalconsciousness]

I'm still in C++ diapers fixing silly bugs like where I used the i from my first for loop in the nested one instead of j.

[@realnapster1522]

Use better variable names. Don’t use i and j for writing loops. Use something like cnt or index.

[@jonforhan9196]

@@realnapster1522 i is a readable convention, but really you should be using iterators or algorithms from the stl

[@assonancex]

This talk was awesome - Fixing my code whilst listening too haha

[@winnie8614]

Why would you want default constructor for RAII object?

[@Dth091]

RE: 29:00 We can have overloads on rvalue-ness, so could we delete the derefence operator on shared_ptr temporaries? T& operator*() && = delete should be enough to add to the class.

[@ericcurtin2812]

Great point made at 50:20... Facebook probably loses a lot more money from bugs that arise from sacficing safety and such trying to make your code super lean in terms of copies. If we cared that much, we would ignore type safety, RAII, etc...... More or less 90% of the features C++ brings and just write everything in C or assembler in order to be super lean... The beauty of C++ as a high-level language is, it's the highest level language around that you can micro-optimize at the C or assembler level IF YOU REALLY NEED TO... 90% of a program execution time is spent in executing 10% of the code... So often only cost you in that 10%... And also often the executing code isn't the bottleneck, it's disk IO and networking... That said everyone should know the basics of when to pass by value, const value, reference, const reference, pointer, but be sensible about it and don't overdo it... And a great response too... Library authors may have to care a little bit more... Their decisions can influence hundreds of thousands of projects around the world, depending on the library...

[@therealjezzyc6209]

Just use Rust and 90% of your comment is invalid

[@anon_y_mousse]

You can write code safely in C, it just requires being a good programmer. C++ makes it harder to be unsafe, but easier to have logical missteps turn into wrong output. C makes a lot of those problems a moot point. Consider whether something will construct or copy, no problem in C, it's a pointer so you only construct it when you want to, you only copy it when you want to, it does nothing you don't mean for it to do and without you needing to tell it not to. R-Value references are one of the most useful additions to C++ and one of the dumbest. std::move() is super useful, but it shouldn't be needed.

[@anon_y_mousse]

@@therealjezzyc6209 No.

[@MrAbrazildo]

4:35, there's a major difference between seen this diagnosed or not. Undiagnosed is something that I don't see for an entire era. So this is not an issue for me. What I made to get rid of this once and for all, several years ago: Made my own custom class, inheriting the container: boundaries checking are applied automatically, giving me in case of error: address, size, wrong index used and even the container name in the project. Pretty easy to catch. 5:35, it's also important to have them checked automatically, under the hood, because filling the code with ifs will be: tiresome, a lost of time, polluting the code, harder to read/maintain, a loss of performance, due to branching - _even permanent, if not embraced by a macro-id, which will pollute the code even more_ . It'll take its toll of ~25% of performance. But all checkings are compiled under a macro-identifier. So, if I remove its #define, I get my performance back at once, Thanos-style. Plus, it's not an often bug to me. Usually, the index is obtained via an algorithm, that returns an iterator, translated to index (if needed). A check if the algorithm failed should be mechanic for any programmer. So, how can it simply jump out of the window? What I can imagine: - A f() took a wrong index, assuming it was checked. I use to make the check right away, after the algorithm. Even if not, the previous tool catches it. - The project demands some mathematics, changing drastically where the index should be applied. It's a periculous case, so the coder should be cautious. And if this happens often, it's better to write a small f(), leaving direct use of [ ]. - Something is changing it externally. It could be the user or some volatile hardware entry. I think the 2 solutions before handle this. - The wrong enum was applied, from other "collection". Just use enum class.

[@blauerBrunni]

Thank you very much for this talk. It again shows how difficult it is to solve easy problems like returning some value from a method is in C++. C++ is too much performance-centered instead of keeping it easy to read and write.

[@nintendoeats]

I had that lock declaration bug. Yesterday. So, less than 24 hour latency being told about the bug!

[@SolomonUcko]

Bug #6 (29:10) seems very similar to the Most Vexing Parse (en.wikipedia.org/wiki/Most_vexing_parse).

[@victorz7748]

Reading one error message from ASAN whole day and fixing a bug from co-worker's code by maybe changing 1 line, is really painful

[@Dth091]

Also RE: std::map -- why can't we have a const overload for operator[] ?? The const overload can be UB if you access an entry that doesn't exist.

[@OMGclueless]

That would be super unsafe, IMO. It's not always clear whether something is const at point-of-use and triggering UB in that case would cause lots of bugs (especially for programmers who have learned over the years that [] on maps always is safe to do without bounds checking).

[@ashtree129]

That seems significantly worse

[@origamibulldoser1618]

Good god, what am I doing with my life.

[@TUMATATAN]

Did anybody else went and checked their "std::unique_lock lk(m_mutex)" code right after this?? That scared the crap out of me.

[@quangtung2912]

@@Lunaticracy If someone use unique_lock, it will probably be used in the next steps. If not, they have already used lock_guard. So your argument is probably not valid.

[@JackAdrianZappa]

At 32:20, *even* if that did produce a lock_guard object, it would be a temporary that would be destroyed immediately after it was constructed.

[@janisir4529]

Well, I'm glad I'm a huge fan of using the T a = T(args); kind of constructor basically everywhere, unless a bracket constructor is convenient.

[@michal.gawron]

10:24 That's called "cargo cult programming" and in my opinion it's the first source of bugs made by newbies in _any_ language. ;-)

[@MrAbrazildo]

5:05, who was the F** that applied a literal on a container? I rarely do this. I can remember on automatic tests or to get the size of the 1st element, when it's the same for the others - _even so, I always use 0, and I'm certain that was allocated at least 1 member_ . 7:38, you mean *professional* newbies. Actual newbies are horrified by == operator and ';'. 7:50, I use to have a global const array of string literals, for communication to the user. And I also have an enum of ids for each. So, I would use m[to_user::hey], getting compiler error if m[to_user::hye]. This is way faster and secure than std::map. But I know that if it's the case of several users "ins and outs", there's no enum that would handle this. The solution is what I said: a company-custom class, inheriting std::map, making all checkings automatically, without being seen by the programmer. 12:15, you can also configure 'operator []' to not mess things. 8:35, no, this is awkwardly Javascript-like, programmer is loosing control of things. Plus, a performance penalty is under the way. 16:08, make this f() call other, inside a subnamespace: namespace dont_call_me { std::map::::iterator get_default (m, k) noexcept; } It would use std::find_if internally, returning an iterator. Then you test it, returning default or not. It would be pretty performatic. And if you are obsessed by the idea of returning a reference, then create a global string called 'defaulty', which can always accept any value, just to serve as a temporary default 1. 18:40, #define volatile _volatile_does_not_make_ // ...write all the text. This will catch volatile as compiler errors.

[@lunedefroid8817]

Guys at my job wouldn't allow using non initialized consts, like map["key"]. We always initialize them, because it won't compile if you make a typo.

[@MrAbrazildo]

I don't have the sensation of having recurring bugs. So it took me some minutes to figure out what could be a list of them: *Ctrl-C + Ctrl-V* _(from my codebase, not Stack Overflow)_ : noob thing? Contrary of what internet thinks, this is good practice. It saves me a ton of subtle tiny energy losses. The price is some bug after a while. Once I timed it: 1 bug after 20 timed hours (~ a week). The worst of them (not related to this 1), took me some hours to solve (not timed). *Things under Construction* : when I write f() or class, I use to not have a clear idea about the details. It's enough to go ahead, but conflicting things happen along the way. They are pieces of code/ideas that mismatches 1 with another. *Conflicting Ideas vs Reality* : sometimes I think about some part of an algorithm or something alike, and then about another, realising the they will work together. So the brain gives the green light. And I implement the thing, but they don't match. So I'm back to the previous situation. *Escaped from Encapsulation* : OO provides safer code, not some weird technique f()-languages could make. Everytime a bug occurs, usually a list of all suspects that may had caused it are already written in the class: its friends, the only ones who can change the variable. But depending on the project, this variable may receive an already wrong value, by another who was affected the same way by another, and so on. So this chain can be large, fading away the encapsulation security. This means you gonna feeling less and less protected, with more things to deal and wrong clues appearing on the way. And the project must be complex enough to raise the bug effect at the ending of this tail, which is hard. Bugs are dumb, not conspirationists. This is not C++ or OO fault. Just means that your project has at least a complex part. Fortunately, this is rare. You can expect it 1x per new project. But this is C++ and its well made encapsulation. Languages like Java/C# would fall badly much earlier, since they don't have it at all: kzbin.info/www/bejne/h36Umoiba66inK8 . And other languages are inspired by them, so I would not wonder if C++ is the only 1 doing it properly. *Misleading Simplicity* : some bugs can stay under the radar, by appearing to be too inoffensive. I use to have a good feeling about bugs, in such way that by the looks of things, I'm directed on track of them quite often. But I was already fooled by those little creatures. They get threatful when are too specific of a private action, inside a f(), and have side-effects, getting hard and demotivated to make a automated test for them. Solutions? Take them off from the f(), just for testing, could lead to worst things, like being called from who shouldn't. Make an entire class for each of this kind, just to make the original f() its friend, seems too much and tiresome. Perhaps the best approach would be to has a new feature in language: f() friend of f(), and restricted to only 1, to encourage take this kind of code out from its original f(), making a new 1, still being private to its origin. It would make easier to write automated tests.

[@JohnWasinger]

6:20 Ada does array bounds check for every single access natively. But it has is no concept of include file guarding.

[@ldmnyblzs]

The big goofy emoji in the thumbnail almost scared me away from this otherwise great talk.

[@maksimivanov5417]

A brilliant talk.

[@anon_y_mousse]

This is an example of a good talk. Definitely will recommend it. I would say the only reason to use volatile is to keep the compiler from optimizing away a variable. In C you have to do all this on your own so it's not uncommon. C++ can just abstract it away, which is one of the nice features of C++. I have never liked the type(thing) form of casting and/or initialization, and it's never passed the smell test for me. I can't say as I like the C method of casting either. While type{thing} is better, I still don't like it. There can be such a thing as too much inference. C++ already had a handy solution with *_cast(thing). Sure it's verbose, but casting probably should be more explicit. As for the std::map issue, yeah, that one irks me which is why I have my own map. Consider that std::vector v; v[0] = 0; is an error, map["key that doesn't exist"] = foo; should be as well. Although in my shadow library I bounds check in debug to catch such mistakes and for release I count and log them.

[@waldolemmer]

*to prevent the compiler from optimizing away a **read or write***

[@kartikmahajan4405]

great talk

[@verylongtrain]

Regarding Bug #3: The solution seemed obvious to me... detect if the default value is a temporary, and either disallow it or make that version return by copy. I went and looked it up in the folly library, and at least now it seem they do exactly that. except they call that get_ref_default() instead: template typename Map::mapped_type get_default(const Map& map, const Key& key); template const typename Map::mapped_type& get_ref_default(const Map& map, const Key& key, const typename Map::mapped_type& dflt); template const typename Map::mapped_type& get_ref_default(const Map& map, const Key& key, typename Map::mapped_type&& dflt) = delete; template const typename Map::mapped_type& get_ref_default(const Map& map, const Key& key, const typename Map::mapped_type&& dflt) = delete;

[@williamchamberlain2263]

30:40 ; go home C++ - you're drunk.

If it's a class method, you can get around the RAII initialization bug by always using "this->" before member variables :-)

[@xrtgavin]

Bug#1：std::vector::operator[]，bound check Bug#2：std::map::operator[]，insert data unintentionally Bug#3：mapGetDefault()，return reference to temporary Bug#4：volatile Bug#5：is shared_ptr thread-safe? Bug#6：mistake variable declaration as definition, RAII + default constructor => DANGER

[@ihormay3418]

Excellent talk!

[@SetMyLife]

How in the world does string(foo) "look like a declaration"?

[@enerjazzer]

You have parens from the old C rules, when you declare pointers to functions and arrays, like int (*pf)(int) or int (*parr)[10].

[@pyajudeme9245]

Great!

[@sinom]

std::optional not supporting references is horrible...

[@kuhluhOG]

41:10 I would just enforce to always use "{" and "}" for constructor calls because that seems to catch it too.

[@MrPAULMUNTEAN]

Louis, thank you for the great insights. I was wondering if ASAN is used at FB and if yes, what other sanitizers/tools have also a large impact on the security of FB's software stack. Thanks!

[@aliancemd]

The std::string/unique_lock issue(32:20), while it blew my mind that this happens :) - it's weird that people wrote that code anyway, it is an rvalue and even if that code would of worked, the destructor is instantly called after the constructor.

[@Sopel997]

Sometimes you just forget to put a name, because you don't pay attention. And it compiles, so you don't look into that even again.

[@ccreutzig]

I just found a std::unique_lock{mutex} lurking in some code …

[@peterkovac2760]

Do you know any linter that has checks against it?

[@danielphd5072]

Thank you

[@kolbstar]

Can anyone explain the bug at 15:34? Wouldn't the code he posted not compile since a temporary string isn't an lvalue?

[@framepointer]

Rvalues are allowed to bind to *const* lvalue references due to how temporary materialization works in C++. You might find this useful: www.open-std.org/jtc1/sc22/wg21/docs/papers/1993/N0345.pdf

[@DirtMcGurtskie]

38:53 -> std::scoped_lock won't let you use a default constructor... Yay! Safe! error: no matching function for call to ‘std::scoped_lock::scoped_lock() /home/prater/src/koala/src/assets/base-asset.cpp: In member function ‘void Koala::Assets::BaseAsset::AddTag(const string&)’: /home/prater/src/koala/src/assets/base-asset.cpp:102:31: warning: unnecessary parentheses in declaration of ‘tagsLock’ [-Wparentheses] 102 | std::scoped_lock(tagsLock); | ^ /home/prater/src/koala/src/assets/base-asset.cpp:102:40: error: no matching function for call to ‘std::scoped_lock::scoped_lock()’ 102 | std::scoped_lock(tagsLock);

[@NyiBBang]

The code at 15:16 is not broken at all, it works fine. I tried something similar under gcc 5.4 and checked it with ASAN and memcheck. Const l-value ref extend the lifetime of objects, and 2 slides later, when he binds it to auto& v, auto is deduced to const string and same thing happens.

[@Predelnik]

It's broken because if call on 15:39 returns reference to default value, its lifetime is not extended since it is not stored in a variable as a result of some function. Here's simple example where it while sadly not crashes but leads to undefined behavior: wandbox.org/permlink/NpcfF87AuXQx5xcU

[@mbabuskov]

It is ok if the default value is a string literal which is mapped to the data section of the executable. He gave a bad example there. Should have used a temporary variable for the default.

[@Mirality]

Actually no, that's still bad. The character literal itself is still intact (as constant string data). The temporary std::string wrapper around it, however, is not. (And this contains a pointer to a *unique copy* of the string literal, not the "real" string literal.) And now you have an invalid std::string&. Had it been a const char*& instead, then you might have been safe -- but then you wouldn't be able to return that from a map of std::string values, so it's not a solution either.

[@mbabuskov]

Ah, of course. Not only does the code crash, it also doesn't do the thing it was used for in the first place (avoiding copy of memory). Thanks for explaining.

[@Mirality]

FWIW, you can make it mostly safe and without copies by making an overload that returns a std::string_view (by value) and passing the default parameter as a const char * . BUT this must be an overload (with another overload that takes const std::string&, to help discourage misuse) and you must promise to only ever call it with string literals, never with method().c_str() or similar silliness (so it's not *completely* safe). And of course in doing this you require C++17.

[@michal.gawron]

12:18 I think a method like vector::value_or (T const&, T fallback_value) const; could be useful if added to the standard. ;-) Or maybe just add "default" value parameter to the ::at() method?

[@jirihavel9766]

The problem with default value for "at" is that "at" returns reference. But the method with default argument should return value, not reference. "value_or" sounds much better

[@NXTangl]

Really, map::operator[] should have returned a reference to the abstract slot. This would be vector-of-bool levels of weird but it would be for all maps so everyone would get to deal with it. You could have implicit conversion throw if empty.

[@RoamingAdhocrat]

11:00(ish), what's gross about using `std::map` to store settings? What's a better approach?

[@SolomonUcko]

A custom `struct` or `class` which would compile-time check the names and allow heterogeneous (non-identical) types.

[@christophrcr]

27:30 - is atomic really correct? I thought the template argument of std::atomic has to be trivially-copyable?

[@HarinduDilshan]

No. I think he was talking about a new type.

[@GeneralBolas]

There is a Concurrency TS class `atomic_shared_ptr`, which wraps the atomic `shared_ptr` functions into a class, though possibly with efficiency gains. That's probably what he was talking about.

[@Mirality]

There's been some talk about taking `atomic_shared_ptr` and making it a template specialisation instead of a separate class, so that you would spell it as `atomic` instead. Having said that, all the current implementations of this that I've seen rely on a global spinlock (or at best, a couple of global spinlocks), which introduces some serious deadlock problems in certain types of applications.

[@blarghblargh]

you said ban operator[]. I say ban std::map. you pretty much never want an ordered map, and an open addressing hash table is often faster than std::unordered_map anyhow

[@MrAbrazildo]

31:08, below, is a variable of type string. Above, is always a temporary, sending foo to its constructor. There's a 3rd possible 1: (std::string) foo, casting a string over foo, which can compile, if foo is a 'char *' for instance. 33:00, a temporary was created. Fix (worked for Clang and GCC, testing on std::string): template using ulock = std::unique_lock; #define unique_lock _deleted_Use_ulock_from_now //Compiler error if used again. #define make_ulock(mut) ulock mut (m_mutex) //Tell your team to create objects only this way. 34:47, again, a temporary was created, this time passing another temporary (controversial) to its constructor. I think the standard is correct, if this improves compiling or runtime performance. And compilers have flag to catch this as 'uneffective code'. 35:18, hm... I didn't know this 1. 38:47, once again you can make your custom-class, inherit unique_lock, delete its default constructor, just to see the bugs, and then reactivate it again.

[@dresnyd]

for the map square bracket operator. perhaps returning some form of std::optional? not sure how well that plays into performance critical or clean looking code tho :D

[@nnicolas17]

I think the big problem is compatibility, making it return std::optional now.

[@JohnDlugosz]

As mentioned in the lecture by someone in the audience, this doesn't work because optional doesn't work with reference types. You want to use square brackets on the left hand side of an assignment! How this is normally done is to return a proxy type that delays the actual work until it detects the context: writing via an operator=, or reading via an implicit conversion.

[@beqa2758]

8:33 it prints 12 in my program

[@emilwallin1176]

you probably forgot to miss-spell it. The first one is "hey" the other is "hye"

[@comradepeter87]

Just get rid of constructors. Use one singular static function per class that generates all your objects.

[@mback3713]

This is why I try to stick with ANSI C for our compiled model production Lingua Franca.

[@jakobryden]

what is the tool he is using to test things? "gawd balt"? thanks :)

[@robinkuzmin]

godbolt.org?

[@Zorrr]

If you're going to be dereferencing a shared_ptr right after retrieving it, why would you make it a shared_ptr in the first place?

[@furl_w]

44:32 "leave [the company]"

[@GeorgeTsiros]

introduction to hating c++, abridged

[@fangjunkuang5061]

I thought you would talk about curiously recurring template.

[@LordXelous]

OMG something useful out of Facebook.... I guess I have to now eat my own hat...

[@superscatboy]

Facebook.h be like: const PersonalInformation& spyOnUser(const user_id_t user_id);

[@privateerburrows]

Bug #1: vector::operator[], what exactly is wrong with it? Because of out of bounds access? What if operator[] throws in case of out of bounds access? Is it still a bug then? If you present something as a bug, shouldn't it be at least mentioned why is it a bug? Or, let me ask a better question, perhaps: When is operator[] NOT a bug? Anyways, I find this talk pretentious and rude from the start, assuming that we all should know why something is a bug to such an extent that we are not worth being told why. This pattern continues throughout his speech. Like what is or isn't "volatile". I think I know exactly what volatile is for: example, a memory mapped hardware timer or range sensor; something that is changing all the time or could change any time, and therefore should not be cached on L3, L2, L1, or even in a register. Am I correct? Again, he moves on without explaining. This is the first cppcon video I'm clicking the thumbs down on.

[@not_ever]

"vector::operator[], what exactly is wrong with it? Because of out of bounds access?" He actually does say that the bug is out of bounds access. "If you want to crash the site, don't check the bounds of your vector". "What if operator[] throws in case of out of bounds access? Is it still a bug then?" The [] operator does not throw. "Anyways, I find this talk pretentious and rude from the start, assuming that we all should know why something is a bug to such an extent that we are not worth being told why." This is a professional conference for C++ developers. As mentioned in the talk out of bounds access is a problem in multiple languages. Assuming everybody knows these things is a fair assumption to make and is not pretentious or rude. The person giving a conference talk is talking to the audience in the room. The fact that the talk is recorded and provided to people on KZbin is a nice bonus for those of us who didn't/couldn't pay to attend. Making disparaging comments about the people giving the talk is rude and likely to discourage people from wanting to give talks in the future.

[@awawus]

15:30 unreal engine's TOptional::Get has this exact problem

[@Theodorlei1]

If you read this: Count how many of these Rust catches by default 😶

[@arturvalente163]

As I'm not familiar with rust at all, could you tell me? Does it do a good job preventing some of these or not?

[@chair547]

Bug 1: array accesses are bounds checked unless you use x.get_unchecked Bug 2: a different api design makes it not a problem Bug 3: the borrow checker will yell at you if you try to do this Bug 4: solved in cpp as well as rust. Bug 5: You can't write to an Arc(shared ptr equivalent) thread safely but rust enforces thread safety at compile time so if you try this the compiler will yell at you Bonus bug: you can't get a raw pointer out of an Arc or Rc without unsafe. Bug 6: Jesus fucking christ what the fuck of course rust doesn't have this cpp what the actual fuck

[@OneEyedMonkey9000]

It would have been nice if he done more “stating the obvious “ , more why. Good talk though

[@ooltimu]

Why does he say that "shared_ptr isn't thread safe" is technically a lie? If only the control block is thread safe then why isn't this an absolute no?

[@pranjalagrawal969]

Because technically, the shared_ptr object is thread safe. Any operation you could do on the pointer itself(like assignment to null), and deref, would be properly synchronized. The object it's pointing to is not, but that never was the question, was it?

[@TruthNerds]

+PranjalAgrawal The shared_ptr object is NOT thread-safe. If you assign to a shared_ptr and e.g. deref it concurrently, that's a data race. Only the control block (and deleter call) are thread-safe. That was exactly ooltimu's point. Indeed, if shared_ptr was thread-safe, then there would be no point in e.g. atomic_shared_ptr: en.cppreference.com/w/cpp/experimental/atomic_shared_ptr

[@pranjalagrawal969]

An object that is thread safe can still have a data race, it really depends on how you use it. For example, if you have a thread safe hashmap, and you concurrently put and get the same key concurrently, that's a race, but the hashmap is still considered threadsafe; the code that accesses it isn't. My point was that all operations that are inherent to the shared_ptr object are thread safe. Any object on the object it references may or may not be.

[@TruthNerds]

"My point was that all operations that are inherent to the shared_ptr object are thread safe. " And exactly that is wrong. Unless you can *prove* the opposite applies to shared_ptr (which of course you can't, because you very wrong), the following quote from the C++ standard holds: "The execution of a program contains a data race if it contains two potentially concurrent conflicting actions, at least one of which is not atomic, and neither happens before the other, except for the special case for signal handlers described below. Any such data race results in undefined behavior." ([intro.races/20]) timsong-cpp.github.io/cppwp/intro.multithread#intro.races-20 Conflicting actions are previously defined as any two or more accesses to a memory location, of which at least one is a write. (For object methods, any calls to a method that is not declared const is considered a write, e.g. shared_ptr::reset()). These paragraphs are written as to allow exceptions to be specified elsewhere in the standard, for example concurrent calls to std::mutex::lock(), on the same instance, are allowed even though it is not declared const: "For purposes of determining the existence of a data race, these behave as atomic operations ([intro.multithread]). The lock and unlock operations on a single mutex shall appear to occur in a single total order." [thread.mutex.requirements.mutex/5] timsong-cpp.github.io/cppwp/thread.mutex#requirements.mutex-5 However, for shared_ptr (only) the following is specified: "For purposes of determining the presence of a data race, member functions shall access and modify only the shared_­ptr and weak_­ptr objects themselves and not objects they refer to. Changes in use_­count() do not reflect modifications that can introduce data races." ([util.smartptr.shared]/4) timsong-cpp.github.io/cppwp/util.smartptr.shared#4 I.e., this confirms what has been said before… the control block is specified to be thread-safe (technically, a control block is an implementation detail, so the wording in the standard is different, but that is what it boils down to). So, you can overwrite one shared_ptr while dereferencing another (e.g.), concurrently, even if they co-own the same object. You can also deref the same shared_ptr many times concurrently, because this is a read-only operation. *But you cannot modify a shared_ptr on one thread, and safely modify or read from the same shared_ptr on another thread without additional synchronization.* This is also much different from your example of the thread-safe hash map, where the result is either of two well-defined results (either the erase happens-before the insert, or vice versa, correspondingly, either the key is present in the map after both threads are done, or not). If you overwrite a shared_ptr on one thread, and read from it on another (e.g. just with * or ->), you cannot expect to get either the old or the new value, you get undefined behavior aka nasal demons, as in the quote given above. A program crash or overwriting "random" memory are just two of many possibilities. en.wikipedia.org/wiki/Undefined_behavior Also, you didn't address what I said about atomic_shared_ptr. Again, if shared_ptr was thread-safe, why would they have bothered to define an atomic_shared_ptr class?

[@Mirality]

People like to claim that "shared_ptr is as safe as int". Which is completely and utterly false. It is actually as thread-safe as two ints. Which is to say, not even slightly thread-safe. (With the single exception of control block reference count updates.) Note that there are some architectures (notably, most desktop PCs) where a single pointer/int _can be_ threadsafe (in the sense that you will always get either the old or the new value, never a frankenvalue) even in the presence of data races -- it's just highly susceptible to accidental mistakes which break it (especially due to compiler optimisations) and it's a really bad idea to rely on it even if you're not writing portable code (so always use locks or atomics). But sadly this is also why "it seems to work anyway" and data races escape detection for so long. It is entirely possible to get a frankenvalue shared_ptr due to a data race.

[@nexovec]

people who remove const be like "now just to remove the 'broken' keyword"

[@jakubkrzesowski6229]

at 32:15 alert("spoiler!"); lock_guard(m_mutex); // this would still cause problems do_the_mutation(); -> the unnamed temporary's ~lock_guard() would release mutex prior to do_the_mutation -> nothing gets synchronized You could fix it right away: lock_guard(m_mutex), do_the_mutation(); -> demonstrate the comma operator evaluation order and full-expression lifetime of a temporary, like a pro -> hear all about commas and their equally evil cousins: the ternary operator and the go-to Fun fact: coincidentally, switching back to unique_lock but keeping the comma would signal a declaration problem, because a comma is not only an operator, but also a separator. C++ is Fun!

[@rich_in_paradise]

How can you say "shared_ptr isn't slow" and then explain that the control block access is thread-safe? It doesn't show up in your profile because the code all gets inlined, but of course it's slower than using a bare pointer because it's doing a bunch of extra work every time you copy it or let it go out of scope.

[@ccreutzig]

Slower than a bare pointer? Yes. Slow, remembering what it does? No. If you don't want the extra functionality, use unique_ptr. If you do want it, well, you will never get the performance of bare pointers.

[@GeneralBolas]

There is a huge difference between "isn't slow" and "is as fast as a raw pointer". Is it slower than a raw pointer if the code doesn't get inlined? Sure. Is that performance difference significant in most use scenarios? No. In hot loops, you shouldn't be transferring ownership of memory, so there's no reason to be copying them.

[@winnie8614]

LOL. I once told in C++ chat that they should use at, and they start throwing shit on me :D And say that nobody uses it, developer should know what he passes. (this was actually about vector, not map)

[@jonforhan9196]

Well yeah the vector literally has the size of it baked in, so ‘at’ makes no sense in most cases. Completely different than map.

[@lonec1777]

Sounds like Facebook should have stuck to PHP.

[@DEBBAH1907]

damn c++ is ugly. even C++ programs who attends a conference aren't sure if a simple code compiles or not.

[@nexusclarum8000]

If you've been coding by simply calling api calls in something like python then tbh you don't really have any meaningful amount of programming experience. No seriously. This isn't elitist bullshit. In a language like python it convinces you that you've built some "ai" that does facial recognition all by yourself in only a few lines of code! No. You didn't. That's the work of an incredibly complex library that is ACTUALLY probably written in that "ugly c++" (Tensorflow for example). You probably have no clue what's actually going on any concrete level at least.

[@davidjulitz7446]

C++ is a complex language and this seems to be an ugly parser corner case. Probably most C++ programmer would not write such Code (possibly only by accident, which is probably the issue here) and therefore it is not easy to see it right away that the compiler will accept it.

[@CharIie83]

std::mapmymap; //use if(mymap.find(enum)!=map::end)mymap[enum];

[@jimwinchester339]

Over-dependence on STL !

[@fredg8328]

STL is crap. It's just a bunch of weird things put together. Most of the time you spend a long time trying to understand how it is implemented, just to find out that it was implemented in the strangest possible way. No language additions or new abstraction could make it safer.

[@edgerique]

So the moral of the story is don't use anything from std?

[@lordadamson]

lol. yes create your own data structures which will suffer from even worse pitfalls :D

[@edgerique]

Adam Zahran Creating data structures is part of a programmers job. And honestly, I've been writing my own data structures and when I do run into problems, I can at least fix mine.

[@MarincaGheorghe]

@@edgerique It seems you work by yourself then ☺

[@edgerique]

@@MarincaGheorghe Sure you can't avoid using the cpp stdlib if your work requires you to do so. But in which field do you not write your own data structures? And have you never replaced something from the stdlib because it was deficient?

[@edgerique]

I mostly said it out as a joke and I know it's within your control to make that decision but you have to admit that if X is causing so many issues, you should stop using X.

[@MsMattness]

*tongue click*

[@Smiteforce]

Good information if you can survive the pretentious tone. He basically talks down to the audience as if we're all idiots.