The Problem, The Culprits, The Hope
The Problem
Bjarne Stroustrup’s keynote speech at CppCon 2015 was all about writing good C++11/14 code. Although “modern” C++ compilers have been in wide circulation for four years, Bjarne still sees:
I’m not an elite, C++ committee-worthy, programmer, but I can relate to Bjarne’s frustration. Some time ago, I stumbled upon this code during a code review:
class Thing{}; std::vector<Thing> things{}; const int NUM_THINGS{2000}; for(int i=0; i<NUM_THINGS; ++i) things.push_back(*(new Thing{}));
Upon seeing the naked “new“, I searched for a matching loop of “delete“s. Since I did not find one, I ran valgrind on the executable. Sure enough, valgrind found the memory leak. I flagged the code as having a memory leak and suggested this as a less error-prone substitution:
class Thing{}; std::vector<Thing> things{}; const int NUM_THINGS{2000}; for(int i=0; i<NUM_THINGS; ++i) things.push_back(Thing{});
Another programmer suggested an even better, loopless, alternative:
class Thing{}; std::vector<Thing> things{}; const int NUM_THINGS{2000}; things.resize(NUM_THINGS);
Sadly, the author blew off the suggestions and said that he would add the loop of “delete“s to plug the memory leak.
The Culprits
A key culprit in keeping the nasty list of bad programming habits alive and kicking is that…
Confused, backwards-looking teaching is (still) a big problem – Bjarne Stroustrup
Perhaps an even more powerful force keeping the status quo in place is that some (many?) companies simply don’t invest in their employees. In the name of fierce competition and the never-ending quest to increase productivity (while keeping wages flat so that executive bonuses can be doled out for meeting arbitrary numbers), these 20th-century-thinking dinosaurs micro-manage their employees into cranking out code 8+ hours a day while expecting the workforce to improve on their own time.
The Hope
In an attempt to sincerely help the C++ community of over 4M programmers overcome these deeply ingrained, unsafe programming habits, Mr. Stroustrup’s whole talk was about introducing and explaining the “CPP Core Guidelines” (CGC) document and the “Guidelines Support Library” (GSL). In a powerful one-two punch, Herb Sutter followed up the next day with another keynote focused on the CGC/GSL.
The CGC is an open source repository currently available on GitHub. And as with all open source projects, it is a work in progress. The guidelines were hoisted onto GitHub to kickstart the transformation from obsolete to modern programming.
The guidelines are focused on relatively higher-level issues, such as interfaces, resource management, memory management, and concurrency. Such rules affect application architecture and library design. Following the rules will lead to code that is statically type safe, has no resource leaks, and catches many more programming logic errors than is common in code today. And it will run fast – you can afford to do things right. We are less concerned with low-level issues, such as naming conventions and indentation style. However, no topic that can help a programmer is out of bounds.
It’s a noble and great goal that Bjarne et al are striving for, but as the old saying goes, “you can lead a horse to water, but you can’t make him drink“. In the worst case, the water is there for the taking, but you can’t even find anyone to lead the horse to the oasis. Nevertheless, with the introduction of the CGC, we at least have a puddle of water in place. Over time, the open source community will grow this puddle into a lake.
It was an inspiring keynote and the video is worth watching; the sense of excitement and of this being a Great Leap Forward was palpable!
(Btw, typo in first code fragment, missing * in vector declaration. If a coder of mine refused the change, he would be a prime reeducation target…)
Ah, thanks for stopping by and pointing out the error in the code Guy. I hope to see you at CppCon next year 🙂
An even more optimal chunk of code would be:
class Thing{};
const int NUM_THINGS{2000};
std::vector things(NUM_THINGS);
Note that I cannot use braced initialization if Thing is an alias for int, or if Thing has an implicit conversion from int.
Good one! Thanks
Where did he plan on putting the matching deletes? If I understand the code fragment correctly, the the Things in the vector are copies of the new-allocated Things, and the pointer to the allocated memory is lost forever as soon as push_back returns.
Wow, you’re absolutely right. This seg faults:
for(int i=0; i<NUM_THINGS; ++i) delete &things[i];
I’d wager that he found that out while trying to plug the leak and used one of the non-“new” solutions.
I am surprised how did he/she manage to write delete statements though. He is creating Thing{} on the fly in heap and de-referencing them. Once out of the loop, you don’t have access to those objects on heap.
If you do need different constructors in the loop (e.g. you’d like to construct Thing{i}), then you’re better off using emplace_back. Oh, and if you’re pushing (or emplacing) a known number of things into a vector, don’t forget to reserve() first; it’s not needed for correctness, but be nice to your allocator.
As already pointed out, there is no way to create a loop of deletes that will fix the problem since the pointers that need to be deleted are lost immediately.
And as others have implied, both the use of new and the use of push_back should draw a lot of questions in a code review. There are better alternatives here, such as using the sized constructor for vector, but in other circumstances new should often be replaced with make_shared/make_unique and push_back should often be replaced with emplace_back. Developers should get used to justifying use of new and push_back with comments in newly written code before it even reaches code review.
Great comments. Thanks.
is it me or it could be also dealt in this way:
class Thing{};
std::vector things{};
std::generate_n(std::back_inserter(things),2000,{ Thing{}});
Sorry that last comment was missing some lambda details:
class Thing{};
std::vector things{};
std::generate_n(std::back_inserter(things),2000,{Thing{}});
It did it again, my point was that it would have a been a lambda that would returned a Thing object.
Actually when someonhe doesn’t understand afterward
its up to other users that they will help,
so here it happens.