[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
----------------
mstorsjo wrote:
> 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.
> 
Actually, scratch that, the appends on line 367 don't use long enough things to hit the allocator.

We could add another case here above (say around line 182) that appends using a plain path as RHS. With your suggestion for `operator/=`, that one succeeds with a `DisableAllocationGuard`, so we could add such a thing here too. Should that go before or after the rest here, though? It feels a bit silly to be adding a new testcase to show off that the header file fix has the intended effect, when the rest of the file (with its existing `DisableAllocationGuard`s) actually is failing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98398