include <new> when std::nothrow is used

[robUx4]

I get errors like this in MSVC:

include\matroska/KaxBlock.h(71,30): error C2061: syntax error: identifier 'nothrow'

Not sure why it works on other compilers. The nothrow was added in ff6e0db2fe677bbd62dcdc4c9ec13e2054c5f0c7 and worked so far. We don't have any other <new> include elsewhere. But it should be included.

[robUx4]

I don't think it's breaks the ABI or API. And it should be in the 1.x branch. Should I do separate PRs to merge on both @mbunkus ? Or one that cherry picks from the other when it's merged ?

[robUx4]

Putting on hold for now. It seems that even when I include it still complains. That's inside another project that has tons of configuration (tenacity). Maybe they're disabling this on purpose...

Would moving the code definition inside the .cpp file break the ABI ?

[mbunkus]

I'm really not sure, to be honest. My usual reference lists the other way as a "don't":

Inline it (this includes moving a member function's body to the class definition, even without the inline keyword).

Ah, under the "do" is the following:

Change an inline function or make an inline function non-inline if it is safe that programs linked with the prior version of the library call the old implementation. This is tricky and might be dangerous. Think twice before doing it.

I'd rather not risk it for the 1.x branches, to be honest.

[robUx4]

OK, I'll try to find a workaround. I still don't understand what's going on... We build test6.cpp with the latest MSVC just fine in the GitHub Actions...

[robUx4]

I see what is going on, the project uses something like this:

#ifdef _MSC_VER
#ifdef _DEBUG
#define _CRTDBG_MAP_ALLOC
#include <crtdbg.h>
#undef new
#define DEBUG_NEW new(_NORMAL_BLOCK, __FILE__, __LINE__)
#define new DEBUG_NEW
#endif
#endif

It may have to do with the fact that the project uses MFC.

[mbunkus]

Ugh. This is terrible. We as library authors must be able to rely on the language's keywords to work as they should according to the specs, and this preprocessor shenanigans breaks that. Not our responsibility to fix.

[robUx4]

It seems that define is even advertised by MS. https://learn.microsoft.com/en-us/cpp/c-runtime-library/find-memory-leaks-using-the-crt-library?view=msvc-170#interpret-the-memory-leak-report

It seems to be using special versions of new to debug leaks (fine). Except there's is no variant with std::no_throw_t. So they can't be used at the same time...

I can't get around this gracefully on our side as there's no "official" define to detect it is on. Since it's a forced include we don't even have a chance to include our headers before it's turned on (which is a good thing when you want to detect leaks). I can just undefine their local define so it uses the actual new. But then that file will lose the leak detection.

On our side we can avoid using std::nothrow in the header. We can use it in the C++ file (libmatroska is built externally). We can't fix it in 1.7.x though.

[robUx4]

I moved the code in the C++ files. Should I make it a PR into master (1.8.0) or the 1.x breaking branch ?

[robUx4]

And the ultimate fix might be to not use nothrow at all and check the exception. KaxBlock throws some exception, so we require exceptions to be enabled anyway.

We might put the fix in the current header.

[mbunkus]

It seems that define is even advertised by MS.

Oh, I hadn't released this was MSVC's own headers that did this. Ugh…

I moved the code in the C++ files. Should I make it a PR into master (1.8.0) or the 1.x breaking branch ?

I was under the impression that the 1x-abi-break-test branch wasn't working properly & failed to detect several cases that we were sure would break the ABI. Is this still the case? if so, there's no real use in merging this change there as we cannot rely on the result.

And the ultimate fix might be to not use nothrow at all and check the exception. KaxBlock throws some exception, so we require exceptions to be enabled anyway.

That would be fine with me & is likely the best solution as it doesn't break ABI.