search

catchorg / Catch2

A modern, C++-native, test framework for unit-tests, TDD and BDD - using C++14, C++17 and later (C++11 support is in v2.x branch, and C++03 on the Catch1.x branch)
https://discord.gg/4CWS9zD
Boost Software License 1.0
18.63k stars 3.05k forks source link

Careless use of StringMaker can easily lead to ODR violations #2298

Open textshell opened 3 years ago

textshell commented 3 years ago

When adding new specializations of StringMaker in a program it's fairly natural to start adding them in a specific compilation unit to get started. If the class is used in any other compilation unit where catch stringifies a value of that type this is a ODR violation which is one of our beloved "no diagnostics required" gotchas in c++. The result is that the use of StringMaker fails to work most of the time when adding it to existing projects without a central place for such things.

Ideally catch would find a way to make this kind of usage not a ODR violation. Maybe something is possible with use of "static" and/or anonymous namespaces. Or maybe it's not possible or too costly.

In the meantime it might be nice to add a "troubleshooting" section in the documentation to remind users that all compilation units that let a specific type come anywhere close to catch need to have exactly the same StringMaker specialization for that type.

jayeshbadwaik commented 3 years ago

The correct solution would be to never have a specialization only in one TU. All specializations should exist in all TUs identically. Otherwise, it is always an ODR violation, irrespective of whether it's detected or not. (Unless you use static)

timblechmann commented 1 year ago

template specializations do not compose well in this case. it would be great to have a more robust stringification mechanism, but from the top of my head i don't see any using some form of internal linkage.

it may be great to have a 'caveat' section in the documentation, though

jan-moeller commented 1 month ago

As someone who has spent days hunting down an ODR violation caused by a missing #include in a completely unrelated compilation unit I can only second this. In fact, I claim it's almost impossible to provide StringMaker specializations without introducing UB into your test executable.

Note that the issue is entirely caused by StringMaker having a default definition. If the primary template was left undefined, then the only way to generate ODR violations was by providing multiple definitions for the same specialization. This is a much more obvious issue than ODR violations between a specialization and the primary template.

Instead of defining the primary template, I propose to branch inside the calling code. For example, the CHECK-macro could generate an if constexpr() which either, if visible, calls the StringMaker specialization, or delegates to a default implementation. This way, if the same specialization is visible in several TUs, this will instantiate the exact same definition. If a TU has no specialization visible, then NO definition is instantiated (currently the primary template is instantiated, with the same symbol as the specialization in the other TU).

Currently, if I #include my StringMaker specialization into some, but not all of my TUs, then my entire test executable is ill formed, no diagnostic required. With the outlined approach, the StringMaker would be used in those where it is #included, and the default printing would be used everywhere else.

This still isn't ideal. I would prefer a compile-time error that tells me I forgot a #include, but that is not achievable while still providing support for non printable types. But I think it's much preferable to the status quo.