[libcxx-commits] [PATCH] D107755: [libcxx] [test] Generalize defines for skipping allocation checks

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM if buildkite is happy. I think the `DisableAllocations` renaming should be done, but if you object, then I don't care to block this over it.



================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp:204
 #else
   bool DisableAllocations = std::is_same<CharT, char>::value;
+#endif
----------------
As long as we're messing with this, is this a good time to change the name `DisableAllocations` to `ExpectNoAllocations`? Here we're not //disabling// anything from happening; we're just setting up what we //expect// to see happen.

Could even
```
bool ExpectNoAllocations = std::is_same<CharT, char>::value && std::is_same<path::value_type, char>::value;
```
to eliminate that `#if _WIN32`, right?


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp:65-68
+    (void)p;
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(p == Buff);
     assert(static_cast<std::size_t>(a) == OverAligned);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > You shouldn't need to alter these lines.
> One has to; as the overridden operator new above wasn’t called, Buff is still null, and new_called is zero.
Oh, I see, we're calling `new` through the fallback but we're calling this `delete[]` directly from line 78. OK then.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp:65-68
+    (void)p;
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(p == Buff);
     assert(static_cast<std::size_t>(a) == OverAligned);
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(new_called);
----------------
Quuxplusone wrote:
> You shouldn't need to alter these lines.
As above: never mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107755