So you (or rather, I) have this legacy code that must be maintained. It has all the classic symptoms:
- it looks like it was written before
const, smart pointers or RAII were a thing; - the author was a big fan of (overly) defensive programming;
- either not much thought went into the overall design, or the design was compromised over time by later extensions;
- there is a lack of (proper) tests.
Tests
Refactoring without tests is just changing sh*t. — Hamlet D’Arcy
Before you start changing anything, I suggest you get proper tests in place if they are not already there. A comment I got at a presentation was: “Yes, but to add tests I need to decouple stuff and add interfaces, and that is also a change, so I can’t add any tests.” True, but you have to start somewhere. A good set of integration tests can be a good start: test what you can, and add interfaces as you go. There is always some risk of making a mistake, but since adding interfaces should not change any behaviour, it is often a change that can be judged correct by inspection. Other changes, such as ‘fixing this minor thing at the same time’, should be carefully avoided.
About defensive programming
Don’t get me wrong: defensive programming can be useful in itself, or even vital in some cases. The gist of defensive programming is “do not trust other software developers to do the right thing” with your code. However, in many cases ‘we’ ourselves are the ‘other software developers’, and if you cannot even trust yourself to use your own code correctly and you add checks everywhere, this comes at a great cost in run time and readability/maintainability. So it stands to reason that we need to strike a balance. And even if we do want to check for incorrect usage, there are tools that can reduce the time and lines of code spent on doing it.
Let’s look at some examples, so we know what we’re dealing with:
// example1: check, check, double check.
#include <assert.h>
#include <iostream>
class Bar
{
public:
void barMethod() { std::cout << "boo\n"; }
};
void foo2(Bar* bar)
{
assert(bar != nullptr);
bar->BarMethod();
}
void foo1(Bar* bar)
{
if (bar == nullptr) return;
foo2(bar);
}
void foo()
{
Bar b;
foo1(&b);
}Let’s look at the motivation first: what goes on in this code? The author of foo1() was concerned that someone could pass nullptr, and decided to check for this condition and do nothing in that case. While this prevents a crash, if passing nullptr is truly a programming error, it effectively cloaks a bug and makes it hard to detect.
The author of foo2() takes a different approach and decides that passing nullptr is a precondition violation, so an assertion is in order. In this case the assertion will trigger in debug builds, but in production nothing will prevent a crash. That sounds even worse, right?
In fact, both methods have the precondition that bar can never be null, and from the way they are called we can be absolutely sure that bar is never null. However, if some future author changes things around, that might no longer be the case. So if bar does become null for some reason (or any other invalid value, for that matter), I want to know about it as soon as possible. I would also like the code to express that this was indeed the intended precondition.
There are several ways to achieve this. First of all, let’s look at references:
#include <assert.h>
#include <iostream>
class Bar
{
public:
void barMethod() { std::cout << "boo\n"; }
};
void foo2(Bar& bar)
{
bar.barMethod();
}
void foo1(Bar& bar)
{
foo2(bar);
}
void foo()
{
Bar bar;
foo1(bar);
}Here references are used to express that bar should always have a valid value. The methods foo1() and foo2() do not check this explicitly, but if bar does become invalid, the code will likely crash. I personally like this style and use it whenever possible. I actually prefer a crash over ignoring the call or getting an assertion, since it exposes the bug early, very likely in a unit test.
Looking back at the first example: if the call is ignored, the bug will be silent and will likely rear its ugly head later in unpredictable ways. Also, both methods only catch the case where the value is exactly null; they will not catch values such as -1, 0xdddddddd or something else.
This is useful to remember, because it can introduce differences in behaviour between debug and release builds. Depending on your environment, in debug builds variables can be zeroed out and memory filled with 0xcccccccc, 0xfeeefeee or 0xdddddddd, depending on whether it was just allocated, initialized or freed.
In some cases the assertion can hang a continuous integration system, because it pops up a message dialog by default. As far as I know, though, this applies to MSVC only.
You might ask: isn’t a crash always worse than no crash at all? I think not. A program that continues to run with unknown behaviour is, in my opinion, much worse than a crash, because it is a potential later crash that will be unpredictable and much harder to debug than the initial problem. If we’re going to crash, we should do it early, so we can catch it before it reaches production.
There are also cases where references are not an option, for example in data structures where the pointer can never be null but must be reassignable to point to a different object. For those cases, the not_null<> template from the GSL seems to work well.
#include <iostream>
#include <gsl/pointers>
class Bar
{
public:
void barMethod() { std::cout << "boo\n"; }
};
void foo2(gsl::not_null<Bar*> bar)
{
bar->barMethod();
}
void foo1(gsl::not_null<Bar*> bar)
{
foo2(bar);
}
void foo()
{
Bar bar;
foo1(&bar);
}This is still pretty good: each method explicitly states that it is the caller’s responsibility to know the pointer is valid, and if it is not, it fails fast by crashing. Both approaches at least document the precondition and allow us to remove the duplicated null checks at every level.
A couple of problems can arise when applying this change. When changing the method prototype to use references, there will be a point where the existing pointer has to be dereferenced to bind it to the reference. You have to be sure the pointer is not null before turning it into a reference, because dereferencing a nullptr is UB. In practice it will probably not crash until the reference is actually used, but this can be tricky (warning, bad code ahead): see this example on Compiler Explorer.
There you can see that checking a reference for null can have interesting side effects: the check is completely optimized out. Fortunately, Clang is nice enough to emit a warning when this happens.
At first glance you might think this is a compiler bug, but it is not. The reasoning goes something like this:
- you cannot bind a reference to a null value (
int& i = nullptr;will not compile); - dereferencing a pointer containing
nullptrcauses undefined behaviour (int& i = *ip;is UB whenipisnullptr); - so a reference can never refer to null;
- which means the
ifcondition can never be true, so the wholeifstatement can be optimized out.
Long story short: (1) make sure the pointer you dereference is not nullptr before passing it to a method that takes a reference, and (2) don’t do any null checks on references; it makes no sense and will have unexpected results.
1: void foo2(Bar* bar)
2: {
3: assert(bar != nullptr);
4: bar->BarMethod();
5: }A side story on the ASSERT case above: it can have interesting side effects on static code analyzers! The analysis goes like this:
- line 3, the analyzer reads: you’re telling me
barcould benullptr; I’ll make a note of that (why else would you check for it?); - line 4, the analyzer reads: you’re calling a method on
bar; let me check whether that could benullptr… err, YES, it could! I’d better warn the user to add a null check!
Removing the assert actually gets rid of this false-positive warning from the static analyzer.
Summary of this section on defensive programming style:
- Some code may rely on the method doing nothing when called with
nullptr, so removing the null check and changing to a reference will in that case introduce a crash at best, and UB otherwise. - Some code may need to reassign the pointer, so references are not always an option.
- Where references can be used, a crash will only occur when the reference is actually used.
not_nullcan be used in both places and can be configured to crash when it is assigned a null value, which is exactly when I want to know about it.
Conclusion: using references where pointers are passed that may never be null is an improvement over raw pointers, since it documents the precondition. But
not_null<>expresses this intent more explicitly and has the added benefit of earlier error detection.
Dealing with raw pointers that manage lifetime
So in C++ we have smart pointers, right? Raw pointers? So passé! NO. Let me be clear: raw pointers are great, always have been, and likely always will be. However, owning raw pointers are, or should be, a thing of the past. An owning raw pointer looks like this:
Foo * foo = new Foo();Do you see the special syntax that makes it very clear that this raw pointer is an owning raw pointer, meaning that the method receiving it is responsible for cleaning up the allocated memory? You don’t see it? Makes sense, because there is nothing that distinguishes this pointer from any other kind of raw pointer.
void f()
{
Foo * foo = new Foo();
g(foo);
h(foo);
}
void g(Foo* foo) {} // non-owning raw pointer
void h(Foo* foo) { delete foo; } // owning raw pointer A piece of advice often heard is:
Do not create owning raw pointers.
But what do you do when you (again: I) have a 15-year-old code base where this happens all over the place?
class Message{};
Message* receiveMessageFromHardware()
{
return new Message();
}
class Engine
{
public:
void process(Message* p) { p1(p); }
void p1(Message* p) { p2(p); }
void p2(Message* p) { p3(p); }
void p3(Message* p) { delete p; }
};Because this is a toy example, it is easy to see that the Message is allocated in receiveMessageFromHardware() and deallocated in p3(). In practice, however, it can be a long and interesting journey through the code to find all the places that allocate Message objects and all the end paths that (should) deallocate them. And if the full path to p3() is not followed because of an error condition, who will deallocate?
The first thing to do is probably just to annotate the code to make clear that the raw pointers are actually owning in this case:
#include <gsl/pointers>
class Message{};
template <typename T>
using owner = gsl::owner<T>;
gsl::owner<Message*> receiveMessageFromHardware()
{
return new Message();
}
class Engine
{
public:
void process(owner<Message*> p) { p1(p); }
void p1(owner<Message*> p) { p2(p); }
void p2(owner<Message*> p) { p3(p); }
void p3(owner<Message*> p) { delete p; }
};This is a great improvement as a first step: it documents and makes explicit that the raw pointers are owning raw pointers. The gsl::owner<> template is very simple:
template <class T, class = std::enable_if_t<std::is_pointer<T>::value>>
using owner = T;That’s all she wrote… It is basically a template that does exactly nothing, except check that the argument is a pointer type. In itself it is a very useful step in documenting ownership, and so in making the code more readable. The assumption that the pointer owns the object, and that the object will therefore either live until the end of the method or be passed on to another method, is now explicitly encoded in the type of the parameter. Note that static analyzers can know about gsl::owner<> and can help you find cases where an owning raw pointer was passed but not deallocated.
The next obvious modern C++ step is, in many cases, to introduce a std::unique_ptr<Message> and move the ownership through the chain, like so:
#include <memory>
class Message{};
std::unique_ptr<Message> receiveMessageFromHardware()
{
return std::make_unique<Message>();
}
class Engine
{
public:
void process(std::unique_ptr<Message> p) { p1(std::move(p)); }
void p1(std::unique_ptr<Message> p) { p2(std::move(p)); }
void p2(std::unique_ptr<Message> p) { p3(std::move(p)); }
void p3(std::unique_ptr<Message> p) { }
};
int main()
{
Engine().process(receiveMessageFromHardware());
}Now the code has improved in multiple ways:
- it is clear about the ownership of the
Messageobject; - correct deallocation of the
Messageobject no longer depends on the error path, early returns or exceptions being thrown.
Written like this, it does not matter whether a function exits early by returning mid-processing or because an exception occurs: the Message object will always be cleaned up correctly, and you don’t have to write any cleanup code to make that happen.
However, introducing std::unique_ptr<> throughout the code can be a challenge. The Message object may be passed through a lot of layers, through a queue, across multiple threads, and so on. What if you cannot, or just don’t want to, change everything in a single changeset?
#include <memory>
class Message{};
Message* receiveMessageFromHardware()
{
return new Message();
}
class Engine
{
public:
void process(Message* p)
{
p1(p);
}
void p1(Message* p) { p2(p); }
void p2(Message* p) { p3(std::unique_ptr<Message>(p)); } // note change
void p3(std::unique_ptr<Message> p) {} // note change
};
int main()
{
Engine().process(receiveMessageFromHardware());
}In this example we only changed the method that does the delete and its caller, and nothing else. This is very nice, because we can now refactor bottom-up without having to change everything in one go. To make the example more interesting, I have added two possible ‘tail calls’ below. It does not matter whether the Message is passed to p3() or p4(); both take ownership and properly deallocate at the end of their execution.
#include <memory>
extern bool IsConditionMet();
class Message{};
Message* receiveMessageFromHardware()
{
return new Message();
}
class Engine
{
public:
void process(Message* p)
{
p1(p);
}
void p1(Message* p) { p2(p); }
void p2(Message* p)
{
if (IsConditionMet())
{
p3(std::unique_ptr<Message>(p));
}
else
{
p4(std::unique_ptr<Message>(p));
}
}
void p3(std::unique_ptr<Message> p) {}
void p4(std::unique_ptr<Message> p) {}
};
int main()
{
Engine().process(receiveMessageFromHardware());
}The approach does not only work bottom-up; here is a top-down example:
#include <memory>
class Message{};
std::unique_ptr<Message> receiveMessageFromHardware()
{
return std::make_unique<Message>();
}
class Engine
{
public:
void process(std::unique_ptr<Message> p) { p1(p.release()); } //notice release()
void p1(Message* p) { p2(p); }
void p2(Message* p) { p3(p); }
void p3(Message* p) { delete p; }
};
int main()
{
Engine().process(receiveMessageFromHardware());
}Here we used make_unique<> to allocate the object, and in process() we take ownership from the unique_ptr and continue passing it ‘the old way’ to p2(). Theoretically, you could even start on both sides at the same time and work towards the middle, but I’m not sure that would serve any purpose.
Const correctness
Another useful thing to document is the intention that a method is not supposed to change anything. In my code base this was not really done (anywhere). The problem with adding const retrospectively is that making a method const probably means you have to make its callers const as well, and this can snowball really quickly. A good way I found to start is to use a static analysis tool (I used ReSharper C++) to tell you which methods could be const (leaf methods) but are not right now. However, I should mention at this point that adding const in an inheritance hierarchy (to any virtual method) can break existing code. This is because const is part of the function signature, for example:
class Base
{
virtual void bar() = 0;
};
class Foo : public Base
{
virtual void bar() const = 0; // by adding const here, still compiles but Foo::bar no longer overrides Base::bar!
};
class Foo2 : public Base
{
virtual void bar() const override = 0; // compiler error! bar did not override any base class methods
};So to deal with this, whenever I added const to a virtual method that overrides anything, I always made sure to also add the override keyword.
After you have done this, re-running the tool gives you the next level of methods to make const. If you repeat this cycle a few (5–10) times, you can quickly improve the const correctness of a project a lot. What’s nice about this approach is that you can stop at any time and the effects don’t snowball, because you started with the leaf methods and are working your way up the call chains.
Smart pointers and RAII types
A custom deleter on unique_ptr can be used to wrap resources:
// ignore (windows plaform stuff)
using HANDLE = void*;
#define INVALID_HANDLE_VALUE ((HANDLE)(-1))
#define PROCESS_QUERY_INFORMATION (1)
#define FALSE (false)
extern void CloseHandle(HANDLE);
extern HANDLE OpenProcess(int dwDesiredAccess, bool bInheritHandle, int dwProcessId);
extern int GetCurrentProcessId();
// ignore (windows plaform stuff)
#include <memory>
struct HandleDeleter
{
using pointer = HANDLE;
void operator()(pointer p) const;
};
typedef std::unique_ptr<void, HandleDeleter> Handle;
void HandleDeleter::operator()(pointer p) const
{
if (p != nullptr && p != INVALID_HANDLE_VALUE)
CloseHandle(p);
}
int main()
{
// now we can write:
Handle handle(::OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId()));
// instead of:
HANDLE h = ::OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId());
CloseHandle(h); // may be needed in multiple branches
}More small things
I found these ‘tricks’ handy:
- create a quick, convenient way to add logging anywhere;
- rewrite typedefs as aliases using the
usingsyntax.
Example code for a Windows-specific way to make adding logging to any class a one-liner:
cdbg << "some value: " << 4 << ", and another: " << 2 << "\n";The cdbg object is a std::basic_ostream<> that writes anything passed to it to the OutputDebugString API. With this approach, a tool like DebugView++ can be used to monitor and filter these messages. If you happen to be on another platform, you can just swap out OutputDebugString for std::cout or your own favourite log function. The point is: an easy-to-use facility for quickly adding some logging can really help you understand code faster.
GitHub link to basic_dbgstream
An example of rewriting typedefs to make them more readable:
// write:
using HANDLE = void*;
// instead of:
typedef void* HANDLE;
// and write:
#include <type_traits>
using FunctionPtr = std::add_pointer<void()>; // declare a pointer to a `void()` function
// instead of:
typedef void (*FunctionPointer)(); // oldschool pointer to a `void()` functionUsing the Dropbox nn<> template
I have not written this part yet; check back later. In the meantime, watch this YouTube video on nn<> to whet your appetite, or see the GitHub repo for nn<>.
What about smart pointers (shared_ptr and unique_ptr) and gsl::not_null?
Earlier versions of the GSL did not allow you to wrap std::unique_ptr<> or std::shared_ptr<> in a not_null<> template, but this seems to work OK now:
class Message {};
gsl::not_null<std::shared_ptr<Message>> msg = std::make_shared<Message>();I need to play with this some more, and will update this post when I do.
What about a code base “infected” with boost::shared_ptr<>?
When smart pointers were ‘new’ and not yet in the standard library (the C++03 era), Boost already had an implementation. Many early adopters realized that using a smart pointer for ownership was superior to using raw pointers. However, in those early days there was no such thing as move semantics yet. As a consequence, many developers adopted boost::shared_ptr<>, as it could be copied easily, and life was good (or so it seemed).
The major drawback of using boost::shared_ptr<>, or any shared pointer, is that there is no explicit transfer of ownership. How long an object lives depends on its last user, and while that sounds reasonable, it encourages sloppy lifetime management, and such programs very often suffer from shutdown problems (crashes).
I have not had to refactor such a project yet, but I expect to take a similar approach as for projects using owning raw pointers: start by nailing down the lifetime of objects, and replace boost::shared_ptr<> parameters with references or not_null<> pointers in methods that do not participate in the lifetime management of the object.
Final words
This post is a work in progress; I will update it once I have more experience applying these techniques, or discover new ones. std::string_view is up next… stay tuned.
References: Twitter thread
Leave a Reply