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

[Martin Storsjö via Phabricator via libcxx-commits]

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
+
----------------
mstorsjo wrote:
> 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.
A second alternative would be to ifdef out parts of the test files; many of these allocations checks are in parts of the test that essentially only test allocations, e.g. doConcatSourceAllocTest in path.concat.pass.cpp, and two of the tests could maybe even skipped altogether, with an `// UNSUPPORTED: windows` tag. That maybe loses a tiny bit of test coverage, although presumably most of the things tested between the allocation tests are tested elsewhere as well.

This approach felt like the least amount of clutter while keeping as much as possible of the rest of the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97497