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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
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));
----------------
Quuxplusone wrote:
> 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`.
Thanks, that does indeed make it less obfuscated!

Unfortunately, it requires moving the `DisableAllocations` variable into an ifdef, but I think the end result still is better.


================
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)
     }
----------------
Quuxplusone wrote:
> 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?
Nice, yes, that does reduce the clutter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107755