[libcxx-commits] [PATCH] D107124: [SystemZ][z/OS][libcxx]: Disable some new operator test cases on z/OS

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a subscriber: libcxx-commits.
mstorsjo added a comment.

(Adding libcxx-commits as subscriber, it seems like this review thread has managed to slip past the mailing list?)

@ NancyWang2222 - We have CI for Windows (both for the DLL and statically linked configurations), so if your patch changes them it should be visible in the CI status. (It won't notice if the patch removes asserts that were passing though - so that's the main thing we need to be on the lookut for.)



================
Comment at: libcxx/test/support/test_macros.h:335-341
+#if defined(__MVS__)
+#define ASSERT_VIA_OVERRIDDEN_NEW(x)
+#define ASSERT_VIA_OVERRIDDEN_DELETE(x)
+#else
+#define ASSERT_VIA_OVERRIDDEN_NEW(x) assert(x)
+#define ASSERT_VIA_OVERRIDDEN_DELETE(x) assert(x)
+#endif
----------------
Quuxplusone wrote:
> Yes, I like how clean this looks! @mstorsjo, how would this play on Windows? Could we replace the Windows testing macro stuff with this macro, as a followup commit?
> 
> @NancyWang2222, I don't think there'd ever be a platform that allowed overriding `new` without `delete`, or `delete` without `new`; therefore, `ASSERT_VIA_OVERRIDDEN_DELETE` is always going to be the same as `ASSERT_VIA_OVERRIDDEN_NEW`; therefore, you should just get rid of `ASSERT_VIA_OVERRIDDEN_DELETE`. Replace all its uses with `ASSERT_VIA_OVERRIDDEN_NEW`.
> (One could imagine renaming the macro to `ASSERT_VIA_OVERRIDDEN_NEW_{AND,OR}_DELETE` to gain technical correctness at the cost of identifier length and readability; but I think `ASSERT_VIA_OVERRIDDEN_NEW` is perfectly clear enough.)
It looks like this is a different thing than D105910...

So on Windows, if libc++ is built as a DLL, then calls to `operator new` within the libc++ implementation cpp can't be overriden in user code. That's what's handled in D105910, and the checks updated there could be shared with the existing exceptions for the Windows/DLL configuration.


But this patch modifies tests that work just fine on Windows - where the user code (the test) both provides the overridden `operator new` and calls it, and that configuration works just fine. (These tests have no `UNSUPPORTED` or `XFAIL` for windows, so the plain `assert()` is passing there.)

So if we make generic macros in `test_macros.h` for it, we need to distinguish between the two cases, as one should be omitted on Windows/DLL but the other one shouldn't. (We don't want to accidentally remove working asserts from some configuration either, as that's reducing test coverage.)


But in this case, the tests we're modifying pretty much only test this one thing (overriding `operator new`), so by removing these asserts we leave next to nothing in the testcases. Therefore, just adding `UNSUPPORTED` in these cases would probably be fine. But in D105910 on the other hand, the `operator new` overriding/counting aspect is only a minor detail of the tests, so it's much better to keep running the whole test and just omit the allocation counting aspect, instead of skipping the whole test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107124/new/

https://reviews.llvm.org/D107124