Find and fix code smells in C++ with regexes

Unify formatting. It makes it easier to write regexes. Suppose you want, for example, to change a type from foo<bar> to foo_bar. If the formatting is not consistent, you might miss places where someone wrote, for example, foo< bar >.

Also having a code formatter ensures that the replaced code can be formatted automatically, this helps to make the whole search-and-replace process easier.

Since you are using a formatter, you could even format temporarily your code to make it easier to write regexes for it, and then format it back. The most useful thing would be to have every statement on a single line. Otherwise, you will miss places that are written on multiple lines. Since formatting the whole codebase might take some time, this is normally not an option.

Writing regexes is already hard enough. Debugging regexes is even harder. Regexes for C++ adds another layer of difficulty. Writing multiline regexes for C++ is something that you do not want to do!

Examples are written for GNU sed ^🗄️ and GNU Grep ^🗄️, without the extended regular expressions flag (-E, -r, --regexp-extended) so some adjustments might be necessary.

Both programs should be available on all platforms, Windows and Android included.

Different programs might use different regex dialects

In particular, you should pay attention to

For example in vim, instead of \b, one needs to use \< and \>, for example :%s/\<NULL\>/nullptr instead of sed -e 's/\bNULL\b/nullptr/g'.

Simply using something like

is not enough, as NULL could be part of another word or macro, like MY_NULL, or be part of a string literal.

Something more robust would be

This way, NULL is not replaced if part of a word ^🗄️.

This fix/enhancement should be one of the first refactorings to be done when refactoring smart pointers. It helps, for example, for a smooth transition from naked pointers and std::auto_ptr to std::unique_ptr and std::shared_ptr where those are useful.

For example std::unique_ptr<int>(NULL); does not compile, but std::unique_ptr<int>(nullptr); does.

Note that simply replacing NULL with nullptr might break some code, for example

but I would argue that this code is misleading, and should be changed (in a separate commit) to

Since std::auto_ptr has been deprecated, and there are better replacements, it should be removed and replaced with something else.

should be sufficient for most cases.

One could argue that

might be better ^🗄️, as it avoids matching something like mystd::auto_ptr_int, but the chances are pretty low.

Other adjustments might be necessary, normally it involves applying std::move where auto_ptr made a copy.

A std::auto_ptr a; can be compared with nullptr (or NULL) through the member function get: a.get() == nullptr.

For std::unique_ptr this is not necessary, as operator== (and operator!=) has an appropriate overload.

Using the member function get is not harmful, but it seems to confuse some developers, some even end up writing a != nullptr && a.get() != nullptr. (One could blame the developers, I think it’s just easier to make the code consistent, remove the function call, and eventually explain the change during a code review).

For simplicity and consistency, removing all the unnecessary calls to get helps to avoid this type of confusion.

This pattern has false positives, as .get() might be a member function of something else returning something comparable to nullptr, and removing it would make the code invalid.

It also does not match nullptr == a.get(). It is more difficult to write an appropriate substitution regex for it, as a simple approach would break expressions like nullptr == foo(a.get()).

A probably good enough pattern is

even if it misses things like nullptr == foo().get().

The pattern should then be generalized also for handling operator!=, the final version looks like ^🗄️:

Instead of writing return std::unique_ptr<int>(); or return std::unique_ptr<int>(nullptr);, it is much simpler to write (and read) return nullptr.

The pattern ^🗄️ could also be applied not only to return statements but to avoid too many changes at once I preferred to handle return statements in separate commits.

Another factor that led me to decide to write the regex this way, is that by replacing std::unique_ptr<int>() with nullptr, it would break code like auto a = std::unique_ptr<int>();.

Note that I’m also matching the ; at the end because I saw too many places that used a construct like std::unique_ptr().get() (true story 🤦).

Those can be adjusted with ^🗄️:

without the need to edit the code by hand.

Note that this transformation might still break or change valid code, for example, foo and bar do not have the same function signature

thus it is not always possible to simply replace std::unique_ptr<int>() with nullptr, even in return statements. For example, the first lambda would compile, but the second not:

What about not nullptr? For example, when using new, like

In those cases, using make_unique ^🗄️ should be the preferred approach.

Note that for new int, using make_unique won’t create a pointer pointing to an uninitialized integer, but a pointer to a zero-initialized integer. So there might be some differences at runtime, for a single int it is negligible, but for more complex structures it could be relevant, and for those cases, std::make_unique_for_overwrite should be preferred.

Thus a possible alternate sed command ^🗄️ is

The interesting part is also \([a-zA-Z :_<>,0-9]*\); a capture group for capturing the type, and paste it in \1. It does not detect all cases, for example, it won’t match A(), where A is defined as #define A() int, or decltype(1+1) but it is good enough.

It is also not strict enough for other situations, for example, int0 can be a type, but 0int not, so [a-zA-Z:_][a-zA-Z :_<>,0-9]* might be more appropriate, but I doubt it has any practical advantages. It would still fail to recognize that a:b or a< are not valid types.

I’ve also left arrays out. The pattern will not, for example, match std::unique_ptr<int[]>. The main reason is that in most cases, a std::vector is a better replacement, and simpler to use, and also because in the codebases I had to maintain yet, I’ve nearly never encountered such a type, so I did not have yet the need to think how to transform such code.

Note that this regex might change the type of a variable.

In this case, the sed command would change the type of a from std::unique_ptr<A0> to std::unique_ptr<A1>.

For supporting such cases, one should prefer a more complex expression ^🗄️:

Similarly, it is possible to create a variant that supports std::make_unique_for_overwrite.

There are now fewer chances to break something, but the code is much more verbose. It might be possible to simplify it with another regex (or create a more complex regex to avoid creating such verbose code), but personally, the previous, simpler, regex has always been good enough™.

Replace snippets like std::unique_ptr<type> value( new type() ); with std::unique_ptr<type> value = std::make_unique<type>();.

Note, while normally I would prefer to use auto instead of typing type twice, the regex does not distinguish between member variables and others.

Since it is not possible to use auto for member variables, the regex will not replace the type with auto

Note how I’ve used [a-zA-Z :_<>,0-9]\{1,\} to ensure that there is a variable, name, oterwise the regex would detect and replace incorrectly std::unique_ptr<type>( new type() )

It is possible to support all listed examples by adding multiple regexes, but I’m going to leave cases like std::unique_ptr<type> value( new type ); out for brevity, as it would double the regexes, or make the proposed more complex.

For supporting the case of no variable name, something like the following command should do the job:

For supporting the example with different types, after the matching types have been handled, can be supported with:

The last use-case is supported with, again, after supporting matching types:

Probably it is best to put them together, to ensure that matching types are replaced before working on different types:

This section assumes that you are mostly using smart pointers with the default deleter.

Otherwise changing a.reset( new int(42) ); to a = std::make_unique<int>(42) ; is a breaking change.

If you are using a custom deleter, i.e something like auto up = std::unique_ptr<a>(ptr, deleter);, consider making the deleter part of the type

This makes it impossible to use by accident the wrong deleter.

For most cases, something like ^🗄️:

should be sufficient.

Using make_unique, like in all other places, has the advantage that it makes it more obvious where possible leaks are.

For cases like a.reset(b.release()) use

It also makes sense to combine it with

If not a.reset(std::make_unique<int>(42).release()); would be left as a = std::move(std::make_unique<int>(42));.

It could be possible to avoid this issue altogether by using

so that this particular code smell is fixed as a separate commit.

The regexes applied for the return statement can be reused in other situations by simply removing return. Similarly, the regexes used for .reset that ends with ; can be reused in other places. Depending on how many matches and particular code constructs, it might make sense to make further changes to restrict the matches and reduce the number of false positives.

Change return new int (42) to return std::make_unique<int>(42).

Further changes are necessary, like changing the function signature and where the function is used. Applying the regex ^🗄️, and compiling afterwards the code, will give an idea of what and how many changes are necessary at least, and eventually discover some bugs.

new is a very common word, so it is useful to try to avoid matching commented-out code and comments, those all lines beginning with / or * (for comments that span over multiple lines).

Ideally, a codebase never uses the delete for freeing memory, as resource management is done through constructors and destructors.

Generally delete cannot be transformed into something useful, it could be deleted. Similarly to new it is a very common word, thus it is a good idea to skip comments to reduce the number of matches ^🗄️

I’m skipping lines that have a = before delete, as those are probably something that looks like a deleted function.

For delete[], use ^🗄️

I prefer handling delete and delete[] differently because often a better replacement has a very different look (mostly std::vector versus a std::unique_ptr).

The Uncopyable class from Scott Meyers Book Effective C++ ^🗄️:

it can help to suppress copy constructors.

I have seen in the wild implementations with virtual destructors, which missed the following advice (emphasis mine)

Even with the virtual destructor, the class seems to work, but it is a pessimization.

The boost implementation ^🗄️ shows that multiple implementations have another possible drawback not described in "Effective C++": unintended ADL.

Another issue is that a subclass might implement a copy constructor. This code (with Uncopyable "updated" to C++11) compiles without errors

Also sometimes users of the class miss following advice (emphasis mine)

Since C++11, it should be preferred to delete the copy constructor (and assignment) operator directly, or follow the "Rule of 0" instead of relying on class hierarchies. Thus the advice of using Uncopyable did (fortunately) not age well. It is generally less error-prone and generally helps little with legibility.

With the following multiline regular expression it is possible to match subclasses of Uncopyable and add deleted copy constructors and assignments:

This regex changes

to

Note that this version does not change, by design, the class in case of multiple inheritance.

Instead of writing

one should prefer the shorter form

Replacing the destructor is "easy", as destructors have a ~ and do not take any parameter.

Note that this regex will not match ~a() override {}, so let’s add an optional capture for override

On the other hand, replacing a constructor is more difficult, as there is no "direct" way to recognize if a given function is a constructor.

Is a(){} a constructor that we would like to set = default;? Yes if it appears directly inside a class or struct named a, otherwise not.

On the other hand, recognizing a non-inline constructor is easier, as the function name coincides with a class name. Thus a::a(){} is a constructor we are interested in, while a::b(){} is not.

sed is able to use a capture group in the expression we want to match, I’m not sure if this feature is widely supported, but it makes it trivial to match outline constructors:

Note that those regexes fail to find functions like a::~a(void){} and a::a(void){}.

It is left as an exercise to the reader to adapt the regexes, even if in my opinion you should stop writing unnecessary void parameters.

Inline constructors are still to be updated, assuming that there will not be many empty nullary functions, searching for (\s*)\s*{\s*}, might be sufficient for finding the remaining constructors without too many positives.

When iterating over a container, it is not uncommon to find code similar too

While there are most of the time more expressive ways to write such loop (with a foreach loop, or with an appropriate algorithm) a little upgrade that can be done with a regex is using auto instead of writing the iterator type explicitly:

The code is generally more compact and less error-prone to maintain.

Note that generally speaking, transforming the first form into the second is not safe.

Maybe c.begin() does not return an iteratortype, and the return type of c.begin() is convertible to iteratortype.

For example iteratortype might be a const_iterator.

Thus blindly replacing one form with the other might have unintended side effects. Even more, if the iterator is compared with something else, after all, begin and end do not necessarily have anything to do with iterators, they could return integers, enum or some other strange type.

For this reason, the presented regex is as strict as possible.

The code matches only if

An alternate approach would be to add a static_assert, and use the compiler to verify if some unwanted conversions have been introduced:

This would transform

to

This means that for the following example

the code will not compile after applying the regex instead of slightly changing meaning, and one can inspect manually what changes are necessary to retain the desired behavior (in this case, use cbegin instead of begin).