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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp:190
+#ifdef _WIN32
+  bool DisableAllocations = false;
+#else
----------------
Aren't there cases where it won't allocate? Even if `CharT` is `path::value_type`? Looking at the code the answer is negative, but I'd like your confirmation.

On a different note.
I've been looking at `operator/=` to which `append` delegates, and we seem to miss an optimization when calling `substr`.

At https://github.com/llvm/llvm-project/blob/main/libcxx/include/filesystem#L1029 and https://github.com/llvm/llvm-project/blob/main/libcxx/include/filesystem#L1034, `__p.__pn_` is a string (`string_type`), so `.substr()` returns a string.
This string gets appended to `__pn_` and immediately discarded.
I think we should do `string_view_type{__p.__pn_}.substr()` and so avoid this temporary string. WDYT?


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp:347
       path LHS((const char*)TC.lhs);
       std::string_view RHS((const char*)TC.rhs);
       path& Ref = (LHS += RHS);
----------------
Here (see below).


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.concat.pass.cpp:373
       path LHS((const char*)TC.lhs);
-      std::string_view RHS((const char*)TC.rhs);
+      std::basic_string_view<path::value_type> RHS((const path::value_type*)TC.rhs);
       const char* E = TC.expect;
----------------
That's a drive-by fix, right? If this is correct, shouldn't line 347 be similar?
BTW, preferably I'd remove this change from this revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98398