[libcxx] [test] Disable allocation checks in class.path tests on windows

[mstorsjo]

Summary

On windows, the path internal representation is wchar_t, and input/output often goes through utf8 inbetween, which causes extra allocations.

MS STL does the same, so this shouldn't be a standards compliance issue.

[curdeius]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp

34–37 ↗

(On Diff #326486)

path::compare should not allocate, nope?
Or is it something else that may allocate in this test?

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp

102 ↗

(On Diff #326486)

These three (ret2-ret4) test compare() overloads that take std::string, std::string_view and const char*, and I'd guess it's understandable that they might do allocations as part of the charset conversions for making them comparable.

We could on the other hand change these inputs to use path::string_type, const path::value_type* and std::basic_string_view<path::value_type> as inputs, and then we could keep the allocation checks.

Do you think that's a better thing to do? As essentially now, the test doesn't really test what it's supposed to on windows, it just does implicit conversions to path inbetween and all 4 cases test the same overload.

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.compare.pass.cpp

34–37 ↗

(On Diff #326486)

If D97551 is ok, I'll update this patch to drop the changes to this file here.

[mstorsjo]

Removed changes to the compare test, after D97551.

[mstorsjo]

Ping

[curdeius]

LGTM but a comment near #define DISABLE_NEW_COUNT (in each test) would not hurt.

[mstorsjo]

In D97497#2606736, @curdeius wrote:

LGTM but a comment near #define DISABLE_NEW_COUNT (in each test) would not hurt.

Good point, will amend it with a comment on each.

[mstorsjo]

Updated with comments added, ping for second approval

[• Quuxplusone]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

32

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?

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

32

This file uses DisableAllocationGuard, which checks that no allocations are done while it's in scope.

[• Quuxplusone]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

32

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.

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

32

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.

[mstorsjo]

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

32

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.

[mstorsjo]

Abandoning this one, D98398 is clearly better, letting us keep a few allocation checks in some files.

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp

 //      path& append(InputIterator first, InputIterator last);
 
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <string_view>
 #include <cassert>
 
+#ifdef _WIN32
+#define DISABLE_NEW_COUNT
+#endif
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 struct AppendOperatorTestcase {
   MultiStringType lhs;

libcxx/test/std/input.output/filesystems/class.path/path.member/path.assign/source.pass.cpp

 //      path& assign(InputIterator first, InputIterator last);
 
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <string_view>
 #include <cassert>
 
+#ifdef _WIN32
+#define DISABLE_NEW_COUNT
+#endif
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 template <class CharT>
 void RunTestCase(MultiStringType const& MS) {

libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp

 
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <string>
 #include <string_view>
 #include <cassert>
 
+#ifdef _WIN32
+#define DISABLE_NEW_COUNT
+#endif
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 struct ConcatOperatorTestcase {
   MultiStringType lhs;

libcxx/test/std/input.output/filesystems/class.path/path.member/path.generic.obs/generic_string_alloc.pass.cpp

 //           class Allocator = allocator<ECharT>>
 // basic_string<ECharT, Traits, Allocator>
 // generic_string(const Allocator& a = Allocator()) const;
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <cassert>
 
+#ifdef _WIN32
+#define DISABLE_NEW_COUNT
+#endif
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "min_allocator.h"
 #include "filesystem_test_helper.h"
 
 MultiStringType longString = MKSTR("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ/123456789/abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ");
 

libcxx/test/std/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp

 //           class Allocator = allocator<ECharT>>
 // basic_string<ECharT, Traits, Allocator>
 // string(const Allocator& a = Allocator()) const;
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <cassert>
 
+#ifdef _WIN32
+#define DISABLE_NEW_COUNT
+#endif
+
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "count_new.h"
 #include "min_allocator.h"
 #include "filesystem_test_helper.h"
 
 
 // the SSO is always triggered for strings of size 2.
 MultiStringType shortString = MKSTR("a");