[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 02:59:09 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;
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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list