[libcxx-commits] [PATCH] D76091: Move more tests to globalMemCounter and reset.

[Dan Albert via Phabricator via libcxx-commits]

danalbert marked an inline comment as done.
danalbert added inline comments.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_array_nothrow_replace.pass.cpp:33
 {
+    globalMemCounter.reset();
+    assert(globalMemCounter.checkOutstandingNewEq(0));
----------------
ldionne wrote:
> EricWF wrote:
> > This change isn't correct.
> > 
> > The test was looking to ensure a *specific* overload of new/delete was replaceable and getting called.
> > It now checks that *any* overload is called.
> > 
> > Please revert this change.
> Wait, how's that true? The only `operator new` that modifies the result of `globalMemCounter.checkOutstandingNewEq` in `count_new.h` is `void* operator new(std::size_t s) TEST_THROW_SPEC(std::bad_alloc)`, which is the same as here, no?
> 
> Other overloads of `new` have different counters, don't they?
I think the problem is that this test is trying to make sure this replacement is called by the implementation if the array variant is not _also_ replaced. Including `count_new.hpp` causes all of the replacements to be used, so the behavior of the actual array new implementation isn't what ends up being tested.

This was in fact covering up a failing test on Android (albeit not one I can do anything about, since we fixed it ages ago but still affects older devices that I support), so the test definitely is doing its job :)

Repushed this in 9c5d0ea6784bf08337bd20d44911ebf6bfbd2822, with these tests fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76091