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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp:153-155
       RequireAllocationGuard  g; // requires 1 or more allocations occur by default
       if (DisableAllocations) g.requireExactly(0);
+      else WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS(g.requireAtLeast(0));
----------------
I would find this easier to reason about if it were like
```
RequireAllocationGuard g(0);  // require "at least zero" allocations by default
#if TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS
if (DisableAllocations) {
    g.requireExactly(0);
}
#endif // TEST_SUPPORTS_LIBRARY_INTERNAL_ALLOCATIONS
LHS += RHS;
```
I'm not thrilled by the macro name `WITHOUT_LIBRARY_INTERNAL_ALLOCATIONS` for two reasons: (1) no `ASSERT` or `TEST` prefix; (2) the associated assert macro is `ASSERT_WITH`, not `ASSERT_WITHOUT`.


================
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);
----------------
You shouldn't need to alter these lines.


================
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);
----------------
You shouldn't need to alter these lines.


================
Comment at: libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.constr/F.pass.cpp:149-150
         }
-        // In DLL builds on Windows, the overridden operators new/delete won't
-        // override calls from within the DLL, so this won't match.
-        TEST_NOT_WIN32_DLL(assert(old_outstanding == outstanding_new)); // (2.3)
+        (void)old_outstanding;
+        ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(old_outstanding == outstanding_new); // (2.3)
     }
----------------
Could we change the "no-op" version of the macro to
```
#define ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(...) (void)(__VA_ARGS__)
```
so that line 149 won't be necessary?


================
Comment at: libcxx/test/support/test_macros.h:417
 #else
-#define TEST_NOT_WIN32_DLL(...) __VA_ARGS__
-#define TEST_ONLY_WIN32_DLL(...) ((void)0)
+#define ASSERT_WITH_OPERATOR_NEW_FALLBACKS(...) assert(__VA_ARGS__)
 #endif
----------------
This PR currently doesn't demonstrate any uses of `ASSERT_WITH_OPERATOR_NEW_FALLBACKS`.

I think you should go ahead and merge D107124 and D105910's changes into this PR, and then ask @NancyWang2222 and @DanielMcIntosh-IBM to download your patch and test it locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107755