[libcxx-commits] [PATCH] D150408: [libc++] Complete refactor of tests for operator new

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 6 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size.replace.indirect.pass.cpp:29
+TEST_WORKAROUND_BUG_109234844_WEAK
+void* operator new(std::size_t s) TEST_THROW_SPEC(std::bad_alloc) {
     ++new_called;
----------------
philnik wrote:
> Are you allowed to override only operator new //or// operator delete? It yes, we might want to add a test for that.
I don't think so, because the memory allocated with one has to be deallocated with the other. They need to use a mechanism that matches. For example, if your custom `operator new` uses `malloc` and the system-provided `operator delete` happens to use `free`, that's fine. But that might not be the case in general, the exact mechanism isn't specified.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.nodiscard.verify.cpp:23
 void f() {
     ::operator new[](4, std::align_val_t{4});  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
----------------
philnik wrote:
> Is there a reason the `[[nodiscard]]` tests aren't merged?
Thanks for the suggestion, I just didn't think of it but it's much better indeed.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp:36
+TEST_WORKAROUND_BUG_109234844_WEAK
+void* operator new(std::size_t s, std::align_val_t a) TEST_THROW_SPEC(std::bad_alloc) {
+    assert(s <= sizeof(DummyData));
----------------
philnik wrote:
> Or is this required?
This is required in C++03 for some tests, but I removed it for others. Same for `TEST_NOEXCEPT` on `operator delete`, I replaced it with `noexcept` when I could.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp:58
+    }
+
+    // Test with a type that is right on the verge of being overaligned
----------------
philnik wrote:
> Do we ever try calling the `align_val_t` overload explicitly?
I added some tests to explicitly call `operator new` for all variants.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150408