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

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

Quuxplusone added inline comments.


================
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
 {
----------------
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.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp:70
     ++aligned_delete_called;
-    std::free(p);
+    DoNotOptimize(p);
 }
----------------
This DoNotOptimize seems unnecessary. I think you can just un-name the parameter.


================
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;
 
----------------
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.)


================
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;
----------------
Here and line 76, the `DoNotOptimize` seems unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105962