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

Marek Kurdej via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Mar 11 02:36:43 PST 2021

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

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list