[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 inline comments.


================
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:
> mstorsjo wrote:
> > 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.
> > Therefore, just adding `UNSUPPORTED` in these cases would probably be fine
> 
> Technically yes; but (1) the state of the art is the awful regex `UNSUPPORTED: target={{.+}}-zos{{.*}}`, so one goal here is to be less ugly and also more clear and granular about //why// the test needs disabling; and (2) it'd be nice to have a solution that "scales" to test files that aren't so simple as these.
> 
> > we need to distinguish between the two cases, as one should be omitted on Windows/DLL but the other one shouldn't
> 
> Agreed. Do you have suggestions for how to refer to these two cases? I'll suggest `ASSERT_VIA_DIRECTLY_OVERRIDDEN_NEW` and `ASSERT_VIA_INDIRECTLY_OVERRIDDEN_NEW`, but I'm not sure I have understood the issue, and if I have, then my suggestions are a little technically/grammatically incorrect. (It's the //call to the overridden new// that is direct/indirect, right? not the //overriding// itself.)
> Technically yes; but (1) the state of the art is the awful regex `UNSUPPORTED: target={{.+}}-zos{{.*}}`, so one goal here is to be less ugly and also more clear and granular about //why// the test needs disabling;

Well I wouldn't let the syntax of that scare away from that solution, if it would be the best one. Also `zos` could be added as one of the identified OSes in libcxx/utils/libcxx/test/features.py like so it'd just be `UNSUPPORTED: zos`, and such a line can be accompanied with a comment explaining why it's there.

> and (2) it'd be nice to have a solution that "scales" to test files that aren't so simple as these.

Yeah total agree on that. My 2 cents here was that in these particular tests, there's very little of value left after disabling this aspect.

> > we need to distinguish between the two cases, as one should be omitted on Windows/DLL but the other one shouldn't
> 
> Agreed. Do you have suggestions for how to refer to these two cases? I'll suggest `ASSERT_VIA_DIRECTLY_OVERRIDDEN_NEW` and `ASSERT_VIA_INDIRECTLY_OVERRIDDEN_NEW`, but I'm not sure I have understood the issue, and if I have, then my suggestions are a little technically/grammatically incorrect. (It's the //call to the overridden new// that is direct/indirect, right? not the //overriding// itself.)

Sounds like you've understood it exactly right to me (that's great!). Yep, wording/naming things concisely is always hard.

I'm not sure `VIA` is the best word, would `FOR` or `WITH` be better do you think? As for how to explain the indirect-call-to-overridden-new correctly in less than 5 words, I don't really know...



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

https://reviews.llvm.org/D107124