Hazard symbol, released under public domain

Use ODR violations in C++ for removing old code

I’ve used successfully function poisoning and deprecated and delete, but lately, I’ve encountered a case where both techniques did not help much.

I had to deal with two classes with the same name, in different namespaces, both used in the same projects.

// old class
// needs to be removed
namespace n1{
  class myclass{
 // ...
 };
};

// shiny new class
// should have replaced n1::myclass long ago
// but because it has a different API it never happened
namespace n2{
  class myclass{
 // ..
 };
};

One class is the old version and needs to be removed. Removing the old class in one go is risky, as it would mean introducing many changes at once, some of which are difficult to test.

I’m a big advocate of iterative approaches. The most efficient way forward was to analyze the code piece by piece and remove the old class from the codebase while development goes further. Adding proxies and adapters would not have helped much, and would probably add a lot of unnecessary code.

Since it is a low-priority task (the old class is not broken, it is just very similar to another one and leads to a lot of code duplication), it happened more than once that it got reintroduced because of copy-paste or transitive dependencies.

Thus the iterative approach without some automated verification helped to clean up only to a certain point. The low-hanging fruits have already been picked, and no one wants to actively change the more complex parts. And those that are less careful might not even realize they are using the old class (because of auto, temporaries, implicit conversion, …​), or simply because they have more important things to pay attention to.

Function poisoning would have helped; except that it is not possible to poison n1::myclass, as it consists of three tokens: n1, ::, myclass, and poisoning myclass would also prohibit the usage of n2::myclass.

= delete helps only if you want to restrict an existing function, not the whole usage of a class, thus it does not help here either. Using could have helped, but I could not mark it as an error, as there are other deprecated APIs that are still in use. One could have parsed the build logs, but it is easy to forget during development. I wanted to have a hard error during compilation.

Use ODR violations to cause compilation failures

After thinking about deprecated and delete I realized I could use a similar technique, although a little more brutal: add another declaration of n1::myclass, and make it unusable.

In my case, as myclass is a value-type class, the following declaration was sufficient:

poison-n1-myclass.hpp
namespace n1{
  class myclass{
    myclass() = delete;
    ~myclass() = delete;
 };
};

With this declaration, if one tries to instantiate n1::myclass, it will get a compiler error, as the instance cannot be created or destroyed "by accident".

I decided to delete the destructor too in case an external function returns a reference which is then copied to a local copy.

But what if one has both declarations in one translation unit? Experience says you get a compiler error if both declarations are visible.

And if you have only the original n1::myclass, then everything works as before.

Mixing in unintended ways by accident both declaration is hard, becase creating an instance of the second class n1::myclass is not as trivial.

Integrate the "poison pill" in your build system

You can include poison-n1-myclass.hpp manually, but it is too error-prone and easy to forget.

Ideally, you’ll be able to remove n1::myclass from a well-defined subset (like a library, or a project subfolder) and use the build system to ensure that it does not get introduced again by accident.

In CMakeLists.txt ensuring that n1::myclass isn’t used anymore in a library would look like this:

if( "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" )
 target_compile_options("${TARGET}" PRIVATE /FI poison-n1-myclass.hpp)
elseif( "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GCC" || "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" )
 # valid for Clang and GCC
 target_compile_options("${TARGET}" PRIVATE -include poison-n1-myclass.hpp)
else()
 # nop if the used compiler does not have something equivalent to "-include"
endif()

If you want to ensure that the dependencies of "${TARGET}" do not use n1::myclass too, then change PRIVATE to PUBLIC.

Disadvantages of this approach

If the file poison-n1-myclass.hpp is forced through the build system on every class as described, then it also applies on all transitive includes, as -include adds poison-n1-myclass.hpp as

Thus` if any header file used by "${TARGET}" (transitive header files included) uses the original n1::myclass, then you’ll have a compilation error.

Note that with function poisoning I did not have the same limitation, as it is valid to use the poisoned token before poisoning it.

With multiple declarations of the class, the second declaration by itself causes an issue, not where the class is used.

The ideal fix is to reduce the usage of n1::myclass further, which is what one wants to achieve, but might require too many changes at once.

One can use forward declarations; instead of having:

#include "myclass.h"

n1::myclass foo();

// other functions

it is possible to write

// no #include "myclass.h"

namespace n1{
class myclass;
}
n1::myclass foo();

// other functions

thus those that want to use foo() must #include "myclass.h" separately. This is generally not good; a header file should "work" out of the box, but it should be only temporarily, as hopefully n1::myclass can now be removed more efficiently from the code base.

The other workaround is to include poison-n1-myclass.hpp, or enlist in the build system the files that should include poison-n1-myclass.hpp unconditionally.

Is the behavior well-defined?

Defining the same class multiple times is well-defined only if all the definitions are the same.

This is not the case with poison-n1-myclass.hpp.

Thus, similarly to deprecated and delete, since the class is only needed at compile-time for causing compiler errors, the way to avoid undefined behavior is to make a separate build or a dry-run to see if there are any errors during compilation.

How do the error messages look like?

The error message with the Microsoft compiler looks like the following:

[build] lib/include/lib/n1_myclass.hpp(29): error C2011: 'n1::myclass': 'class' type redefintion
[build] poison-n1-myclass.hpp(29): note: see declaration of 'n1::myclass'

With GCC, the error message provides more information:

In file included from lib/src/file1.cpp:14
In file included from lib/src/file1.hpp:10
In file included from lib/src/file2.hpp:4
lib/include/lib/n1_myclass.hpp:29:25 error: redefinition of 'myclass'

Thus while this approach works with GCC, clang, and MSVC, the experience in MSVC is terrible, as the compiler does not give you a lot of context. You need to determine by hand the transitive includes to find which file includes n1_myclass.hpp.

So far my approach has been to use GCC or clang, clean the mess up, and then use MSVC.

It helps to remove unused headers, as it makes analyzing the build error with MSVC easier.


Do you want to share your opinion? Or is there an error, some parts that are not clear enough?

You can contact me anytime.