[libcxx-commits] [PATCH] D98398: [libcxx] [test] Disable allocation checks in class.path tests on windows

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/support/test_macros.h:378
+#define NOT_WIN(...) __VA_ARGS__
+#endif
+
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > I think this macro's name needs to start with `TEST_`.
> > Orthogonally, I think `WIN` should be `WIN32`, for greppability.
> > So perhaps `#define TEST_NOT_WIN32(...)`? I wanted to say `TEST_EXCEPT_ON_WIN32`, except that I think someone might misread "EXCEPT" as having something to do with C++ exceptions.
> > 
> > Alternatively, introduce a specific type `DisableAllocationGuardExceptOnWin32` and use that instead of `DisableAllocationGuard` in certain places.
> > 
> > (Really `DisableAllocation` was a bad name in the first place — it doesn't //disable// the allocation guard, it //is// the allocation guard! — and what's meant is more like `ExpectNoAllocations`, `ExpectNoAllocationsExceptOnWin32`. Blech.)
> I kinda intentionally left out the "test" prefix of the macro, designing it similar to the `LIBCPP_ONLY()` macro, but inverted. And it's not so much a "test this"-macro but more generically just a more convenient ifdef. (In some cases, a macro like `IF_WIN32_ELSE(win, else)` macro also would be convenient, but probably not worth it, and it'd be less clear.)
> 
> `WIN32` for greppability sounds good.
> 
> Agree about `DisableAllocation` being a non-ideal name, `ExpectNoAllocations` sounds good. I'm not entirely sold on making an ExceptOnWin32 variant though, as the exact condition for where/when to disable things (ie require that no allocations are done) varies, see e.g. the current modifications in string_alloc.pass.cpp.
> I kinda intentionally left out the "test" prefix of the macro, designing it similar to the `LIBCPP_ONLY()` macro

I do like analogies to existing code... but... look at //literally every other macro// in "test_macros.h"! They either begin with `TEST_` (`TEST_HAS_NO_EXCEPTIONS`, `TEST_STD_VER`, `TEST_CONSTEXPR_CXX14`, `TEST_SAFE_STATIC`, `TEST_IGNORE_NODISCARD`) or their names are arguably specific to testing (`ASSERT_NOEXCEPT`, `LIBCPP_ASSERT`) //or// it's the `LIBCPP_ONLY` macro, which feels like it just snuck in next to the various flavors of `LIBCPP_ASSERT` and nobody noticed. 😛
So I think starting with `TEST_` needs to be part of any macro-based solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98398