[C++ runtime] Fix static const ODR violations

[nburles]

Defining a static const member in a header breaks ODR, as the object will be defined in every translation unit that includes the header.

Instead, the members should either be declared inline (which is implicitly the case for constexpr members) or initialized in a cpp. Alternatively, using an anonymous enum allows the definition to remain inside the header meaning that the compiler can still choose to inline values.

[mike-lischke]

@nburles All you changed were anonymous enums before, so what does your patch improve?

[nburles]

@mike-lischke You mean these bits? e.g.

EMPTY_RETURN_STATE = static_cast<size_t>(-10), // std::numeric_limits<size_t>::max() - 9; doesn't work in VS 2013

I'm happy to change them to remove the static casts and any reference to VS2013 in the comments?

[mike-lischke]

@mike-lischke You mean these bits? e.g.

EMPTY_RETURN_STATE = static_cast<size_t>(-10), // std::numeric_limits<size_t>::max() - 9; doesn't work in VS 2013

I'm happy to change them to remove the static casts and any reference to VS2013 in the comments?

Yes, that was the original thought I had when I spoke of Visual Studio, but then I remembered this is also compiled on other platforms :-)

[nburles]

True... the C++20 version will definitely work on any compiler which supports C++20 - but it could be safer to leave the pre-C++20 version as it is for now then? (Unless there's a CI system which uses each of the supported toolchains?)

[mike-lischke]

We definitely need to keep pre-C++20 support. Currently we even guarantee C++11. That needs to be carefully weigh up when to require C++17 as minimal version. I would love to get more feedback on that from the community.

Can you also say a word about why you changed the enums to constexpr since that seems to make no difference (unless I miss something here)?

[nburles]

Without this change, the original code was e.g.:

static const size_t EOF = static_cast<size_t>(-1);

If the header file containing this line is included in more than one translation unit (very likely!) then it introduces undefined behaviour. Each translation unit ends up with its own copy of EOF, causing linkage problems.

With C++20 (guarded by #if __cplusplus >= 201703L), this is an easy fix - adding constexpr (or alternatively inline) as such:

static constexpr size_t EOF = std::numeric_limits<size_t>::max();

ensures that there is only a single copy of EOF.

Pre-C++20, constexpr statics did not exist, so there are two alternative fixes (both are fine in C++11):

1. Move the initialization into a translation unit, e.g.:
   IntStream.h: static const size_t EOF;
IntStream.cpp: IntStream::EOF = static_cast<size_t>(-1);
   But - this is firstly a pain, and secondly risks running into the "static initialization fiasco". So instead I went with:
2. Replace the static const fields entirely with an anonymous enum. This does not change how the constants can be used, but it fixes their initialization, so they again only exist in a single place.

[mike-lischke]

@parrt This C++ patch is ready to be merged.

[parrt]

Thanks for all good work guys.