[libcxx-commits] [PATCH] D105962: [libcxx] [test] Fix mismatches between aligned operator new and std::free

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp:47
+
+void* operator new [] (std::size_t s, std::align_val_t) TEST_THROW_SPEC(std::bad_alloc)
+{
----------------
ldionne wrote:
> I don't think we need `TEST_THROW_SPEC` at all, since this isn't tested in C++03 mode.
> 
> Same goes for `TEST_NOEXCEPT` -- it can just be turned into `noexcept`.
Ok, I'll simplify those while touching the code.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp:61
 
-void operator delete(void* p, const std::nothrow_t&) TEST_NOEXCEPT
+void operator delete [] (void* p, const std::nothrow_t&) TEST_NOEXCEPT
 {
----------------
Quuxplusone wrote:
> Your commit summary mentions that the old test relied on
> 
> > the fallback from `operator delete[]` to user defined `operator delete`
> 
> Are you sure that this was intentional, and not just a typo in the test? (Personally I didn't know that any such fallback rule existed! And wouldn't that be more of a compiler issue than a libc++ issue, anyway?)
> I'd feel better if we did the `delete->delete[]` renaming in one commit/PR and then whatever Windows fixup is still needed, in a separate PR.
I presume it was just a typo in the test, yes. (There's such a fallback, but we shouldn't be testing that implicitly everywhere but only in a test set out to specifically test that, yeah, as not all systems might support it.)

@ldionne WDYT, keep the `delete` -> `delete[]` change here or split to a separate patch?


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp:33
 
 constexpr auto OverAligned = __STDCPP_DEFAULT_NEW_ALIGNMENT__ * 2;
 
----------------
Quuxplusone wrote:
> Any intuition for whether this use of `__STDCPP_DEFAULT_NEW_ALIGNMENT__` is correct, or if it should just be `alignof(std::max_align_t)`? (Related to my request-for-investigation on D105905.)
I'm not entirely sure about the finer details about which one matters where, but I think the most correct one here might be something like `std::max(__STDCPP_DEFAULT_NEW_ALIGNMENT__, alignof(std::max_align_t)) * 2`, to make sure it really is more aligned than what otherwise is provided. (But this is unrelated to the patch at hand here anyway.)


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp:51
+    void *Ret = DummyData;
+    DoNotOptimize(Ret);
+    return Ret;
----------------
Quuxplusone wrote:
> Here and line 76, the `DoNotOptimize` seems unnecessary.
I guess so yeah. FWIW I copied inspiration for these from new_align_val_t_replace.pass.cpp in the same dir, where you might be able to remove some such extra calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105962