[libcxx-commits] [PATCH] D128864: [libc++] Fix algorithms which use reverse_iterator

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:48
+  auto __reverse_range = std::__reverse_range(std::ranges::subrange(std::move(__first), std::move(__last)));
+  auto __ret           = ranges::copy(std::move(__reverse_range), std::make_reverse_iterator(__result));
+  return std::make_pair(__ret.in.base(), __ret.out.base());
----------------
var-const wrote:
> Question: why doesn't this use `__copy`? (I presume there's a reason, just checking)
Just because it doesn't really make a difference and using the public interface leads to less refactorings when changing the `copy` internals.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:429
+  _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const _AlgRevIter& __lhs, const _AlgRevIter& __rhs) {
+    return __lhs.base() == __rhs.base();
+  }
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > philnik wrote:
> > > > var-const wrote:
> > > > > philnik wrote:
> > > > > > var-const wrote:
> > > > > > > Hmm, so this expression is not ambiguous, but when it's used as a constraint in a `requires` clause, it is considered ambiguous? Am I missing something?
> > > > > > No, that's exactly the case.
> > > > > Interesting. Why is that?
> > > > I don't know exactly why, but I think it is because that would change the overload resolution. With the change in SFINAE contexts you could end up calling different functions, not just allow code that would otherwise be an error.
> > > It looks like this paper aims to fix the language issue: https://github.com/cplusplus/papers/issues/1127
> > > Unfortunately, even though the paper was approved this February, it doesn't fix all the potential cases that create ambiguity, so it won't change anything for us even after it's implemented in the compiler (or at least that's how I read it).
> > First of all, I think the current clang behaviour is really confusing.
> > See this example https://godbolt.org/z/v767Kdd7K
> > - If you write `foo1 == foo2`, it actually compiles fine with just a warning
> > - If you write a concept that checks `foo1 == foo2` is valid expression, it returns `false`
> > I think clang contradicts itself.
> > 
> > 
> > @var-const Thanks for posting this paper P2468R2, I actually think the paper fixes our issue. The example in the paper section 1.2.1
> > https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2468r2.html#open-source-sampling
> > is exactly what we are facing in robust_against_cpp20_hostile_iterators.compile.pass.cpp
> > https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/algorithms/robust_against_cpp20_hostile_iterators.compile.pass.cpp#L50
> > 
> > They both have CRTP base with a member `==` that takes a derived class. And the usage of `derived == derived` is ambiguous due to the fact that both sides are equally convertible from derived to base.
> > 
> > The straw poll of this paper will happen next Monday. I am not sure what is the best action for us for llvm-15
> @huixie90 Yes, this is the part that confuses me. I understand why Clang sees an ambiguity there, but I don't understand why it behaves differently in two different contexts. If Clang wants to be pedantic about this issue, it seems like it should reject `foo1 == foo2`, not issue a warning. If Clang wants to be permissive, it should evaluate the concept to `true`. Of course, it's possible that I'm missing something, but it looks like it might be unintentional.
> 
> Re. the proposal -- if I'm reading it correctly, it's only proposing the `Second resolution`, which is to avoid rewrites if the class has both a user-provided `operator==` and a user-provided `operator!=`. A valid iterator can still define a "hostile" `operator==` without defining an `operator!=`, which would still lead to the same issue we're dealing with. In the proposal, it says that 8 out of 110 projects they evaluated are still failing after the proposed fix. Unfortunately, from our perspective it means that the proposal doesn't really change anything. Even if the number of iterators considered hostile is reduced 10x, it's still a valid use case that we have to support.
> 
> 
I think clang does that, because allowing it in evaluated contexts only allows a program to compile that would otherwise not be a valid program. Having `requires(T lhs, T rhs) { lhs == rhs; }` return true would change the meaning of the program, not just let a program compile. IOW, I think Clang is actually the only conforming compiler [in this example](https://godbolt.org/z/efWM3v5Ea). I could be completely wrong though.

Regarding the proposal, I think it does solve our issue. I would guess that Clang, GCC and MSVC drop the extension after implementing the paper, meaning that it would be an error anyways and thus not be a problem for us anymore.


================
Comment at: libcxx/test/libcxx/iterators/predef.iterators/_AlgRevIter/reverse.iter.cmp/equal.pass.cpp:9
+
+// <iterator>
+
----------------
var-const wrote:
> The new reverse iterator is only used in C++20 mode and above, right? Would it make sense to mark these tests as only supporting recent language versions and simplify them (replace `TEST_CONSTEXPR_FOO` with `constexpr`, etc.)?
I think it's not a bad idea to check that the iterator works the same in all language versions. Only running the tests in C++20 and 23 would simplify them a bit, but not that much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864