[libcxx-commits] [PATCH] D113161: [libc++] Implement P1989R2: range constructor for string_view

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/filesystem:1510
   friend _LIBCPP_INLINE_VISIBILITY bool operator>=(const path& __lhs, const path& __rhs) noexcept {
-    return __lhs.compare(__rhs) >= 0;
+    return __lhs.__compare(__rhs.__pn_) >= 0;
   }
----------------
(FWIW, I think you were right that you //could// use `__lhs.compare(__rhs.__pn_)` instead of my suggested `__lhs.__compare(__rhs.__pn_)`; I just prefer `__compare` because it's one less layer of indirection.)


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/range_concept_conformance.compile.pass.cpp:13-16
+// This test fails due to atomic constraints caching on clang-cl. Specifically, the constraints of
+// std::ranges::range concept changes over the lifetime of this TU. Similar story with libc++ and MinGW.
+// XFAIL: target={{.+}}-w64-windows-gnu
+// XFAIL: msvc
----------------
This sounds sketchy to me. Can you or @mizvekov explain further? (Explain in a thread here, that is. The code comment can be amended, if needed, after I get it :))

This test just includes a couple of libc++ headers and then tests some properties of `fs::path`. If this simple stuff doesn't work, then that smells like a libc++ bug (still), not something for the end-user to work around. In particular, this smells like we still have the same kind of issue as in `__lhs.compare(__rhs)`, but with some other member function, and in a codepath that gets compiled only on Windows... no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161