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

Martin Storsjö via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Mar 11 03:21:25 PST 2021

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;
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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list