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

[Joe Loser via Phabricator via libcxx-commits]

jloser marked an inline comment as done.
jloser 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;
   }
----------------
Quuxplusone wrote:
> (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.)
Yeah, we can call the public `compare` member function. But calling `__compare` avoids overload resolution (see, I read your blog post!), so we can do that to be slightly cheaper.


================
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
----------------
mizvekov wrote:
> Quuxplusone wrote:
> > 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?
> I don't think caching is any different between clang and clang-cl.
> 
> I think the initial issue, from what I read about it, has to do with concepts satisfaction caching and how clang does not implement any kind of invalidation of the cache.
> 
> This could be tested by passing frontend command line switch `-fno-concept-satisfaction-caching`, or if using the driver, `-Xclang -fno-concept-satisfaction-caching`.
> 
> But the difference in behavior between clang and clang-cl must be something else.
I'll precursor my comment with the fact that I don't have a great way to test the `clang-cl` or `MinGW` failing builds locally. It was failing for similar reasons with atomic constraints caching on CI. I think it's a similar issue with the `__lhs.compare(__rhs)` as you mentioned that is only a codepath for when `_LIBCPP_WIN32API` is defined.

I've reproduced the issue locally with `ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_WIN32API=1` to emulate a "windows only code path" that this error shows up. I'll try debugging it soon to figure out the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161