Forward-declaring identifiers in std:: is UB - consider including some of the cheaper C/C++ stdlib headers

[dimztimz]

I noticed that you redefine some stuff that is already part of C and C++ standard library.

For example:

1. You wrap memcpy inside my_memcpy
2. You redefine nullptr_t (which is UB)
3. You forward declare some iostream stuff (again UB).

cstddef and cstring are two headers that are EXTREMELY common. I can't think of a single non-trivial program that does not already includes these two headers. Just including C++ string or vector brings in these two. So, if some client code has already included them, and Doctest includes them, the second inclusion is practically a no-op because of header include guards.

Therefore, when redefining this stuff, you are optimizing the compile time for the best case, and that is using doctest alone, but you are worsening the common case, which is using doctest to test some other software.

[dimztimz]

If you work on linux, consider the following 2 source files.

// a.cpp
int main()
{
}

//b.cpp
#include <iosfwd>
#include <cstring>
#include <cstddef>

int main()
{
}

Now run some timings with

time g++ a.cpp
time g++ b.cpp

You will notice at most 20ms difference, usually 10ms.

On the other hand, if you include <string> you have noticable difference of 200ms.

[onqtam]

You are absolutely correct about cstddef and cstring. The only reason I'm avoiding them is probably... the sort-of synthetic benchmarks (synthetic in the sense that any realistic C++ project will end up including those headers so doctest should be free to use them as well).

I've thought about this aaand I'm still not considering including them - that is until that UB actually manifests itself in some problematic way. This is perhaps the "safest" type of UB and I know anyone pedantic about the standard won't like this choice... But that is what I have that huge build matrix on the CI!

And for the iostream stuff - I could include <iosfwd> as I do when clang & libc++ are used but some platforms have really messy includes - for example on MSVC <iosfwd> drags <cstdio> and a bunch of other things... But there is also this config option!

https://slides.com/onqtam/2017_cppcon_doctest#/49

(the note about boost.DI from that slide is no longer true...)

For doctest 3.0 I might move to modules and import the symbols I need.

I'm keeping this issue open just for reference.

[dimztimz]

This is a showstopper issue for me. Lets say that some implementation decides to use inline namespace inside namespace std with some update. Bam, the code stops compiling.

[dimztimz]

for example on MSVC <iosfwd> drags <cstdio>

I don't this is a problem. cstdio is cheap too. All C headers are, because they are just a bunch of declarations.

[onqtam]

Inline namespaces - that is actually the case with libc++ on clang - so when clang is used I first include <ciso646> just to be able to check if _LIBCPP_VERSION gets defined (which means using libc++) and in that case I proceed with including <iosfwd> for the ostream stuff. So perhaps I should extend that line of thought for the other thing from std:: which I forward declare - nullptr_t.

cstdio is indeed relatively small as well... but there are other headers which also get dragged along with <iosfwd>. I guess I'm clinging onto the ability to have those drastic results in the benchmarks page. Perhaps I should rethink this because it is indeed a code-smell. And doctest has already made it's name as the fastest framework - perhaps I could let some milliseconds of parsing time for the header slip by... You make me think of important things even though I feel bad about making the right decisions :D

So far I assumed that when such an issue arises with libstdc++ or the MSVC stl I could switch to the proper headers being included and just push a new version of doctest. So far it seems that GCC 9 and VS 2019 won't cause such trouble. But on the other side - users would feel annoyed if after updating their toolchains they end up having to update their third party dependencies as well...

I'll think about this a bit more.

EDIT: It is really appealing to have 1 thing less to worry about - I might play with this in a few days and do the benchmarks with a few includes

[dimztimz]

I have provided the patch, merge it if you want.

Correctness is more important than speed. I'll mention again, real world use cases don't lose a millisecond, only the synthetic benchmark.

[zhihaoy]

This is a showstopper issue for me. Lets say that some implementation decides to use inline namespace inside namespace std with some update. Bam, the code stops compiling.

This is not going to happen due to ABI break. For implementation that has used inline namespace already, we can still predeclare these names by looking at which implementation this is: a36f6b8
On the topic of UB, we never count it as UB if it happens inside a standard library implementation, right? Then let's treat doctest a part of standard library. We can file a bug report when it breaks, rather than when it hypothetically breaks.

[onqtam]

I just closed the PR related to this and the reasoning is there in the final comment: #197 (comment)