[libcxx-commits] [PATCH] D97497: [libcxx] [test] Disable allocation checks in class.path tests on windows

Wed Mar 10 12:10:39 PST 2021

mstorsjo added inline comments.

================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp:32
+#define DISABLE_NEW_COUNT
+#endif
+
----------------
Quuxplusone wrote:
> mstorsjo wrote:
> > Quuxplusone wrote:
> > > I'm actually confused about this line's intended effect. AFAICT from a quick skim, `DISABLE_NEW_COUNT` causes `MemCounter::disable_checking = true` which causes all the functions of the form `MemCounter::checkFooBarEq(x)` to automatically succeed.
> > > However, this specific test doesn't seem to contain any calls to `MemCounter::checkFooBarEq(x)`! It's all using `malloc_allocator`, which is unaffected(?) by `DISABLE_NEW_COUNT`.
> > > 
> > > Thoughts?
> > > 
> > This file uses `DisableAllocationGuard`, which checks that no allocations are done while it's in scope.
> Okay, that's a good answer to my question. I'm still unsure whether we should be using `DISABLE_NEW_COUNT` globally like this, or perhaps doing some localized fix such as putting `#ifndef _WIN32` around each guard variable declaration. So I don't object to this fix but I also feel like I'd like to see someone else's opinion/approval rather than making me be the second approver.
Ifdefs easily become unwieldy, but I could add something like a `NOT_WIN()` macro that one could wrap them in, if others also would be ok with such a pattern. It's a bit more clutter, but more precise.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97497