[libcxx-commits] [PATCH] D98398: [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.append.pass.cpp:190
+#ifdef _WIN32
+  bool DisableAllocations = false;
+#else
----------------
curdeius wrote:
> 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?
Thanks for the suggestions about optimizations - yeah that could help (and would avoid one temporary allocation I presume?).

Regarding this particular case, both parameters to `operator/=` and `append()` are of type `InputIter`, which are pass through an intermediate `path` object, see https://github.com/llvm/llvm-project/blob/main/libcxx/include/filesystem#L1039-L1051.

However, the append that we do on line 367 (with RHS being a path), doesn't do allocations, so we could add such a guard there. That seems to run successfully even with the current form of the header.



================
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;
----------------
curdeius wrote:
> 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.
It's a separate fix to make it not require allocations, yeah, otherwise we'd need to disable the allocation guard below. Sure, I can split it out for clarity. I guess we could change line 347 too - there's no allocation guard there so it works either way, but having it use the path native type probably is closer to the original intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98398