[libcxx-commits] [PATCH] D127577: [libc++] Fix std::lower_bound with C++20-hostile iterators

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__iterator/distance.h:13
 
+#include <__algorithm/generic_algorithm_helper.h>
 #include <__config>
----------------
EricWF wrote:
> I would love a more descriptive name for this header.
Do you have a good suggestion? I think `namespace` is a lot worse than what I've got currently.


================
Comment at: libcxx/include/__iterator/distance.h:109
 typename iterator_traits<_Iter>::difference_type __ranges_distance(_Iter __first, _Sent __second) {
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
+  if constexpr (__ns == _Namespace::_Ranges) {
----------------
EricWF wrote:
> Can we lose this `#ifdef`, since I assume we are not passing `_Ranges` when we don't have complete ranges?
> And if we are passing `_Ranges` we shouldn't be silently falling back to `std::`
`ranges::distance` has to exist for `if constexpr` to be happy. I've added a `static_assert(__ns == _Namespace::_Std)`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/lower.bound/lower_bound.pass.cpp:25
+
+template <class Iter>
+concept HasLowerBound = requires(Iter iter, Iter sent) { std::lower_bound(iter, sent, 0); };
----------------
EricWF wrote:
> Can we write this without concepts so we can test it in all the dialects it affects?
> 
> Also, I don't think this tests anything. It's my understanding that concepts can't detect failures outside of the immediate context. So because the call itself is well formed, this test will always pass.
> 
What exactly do you mean? C++20 is the only dialect where it is interesting AFAICT. Otherwise we always call `std::advance` and `std::distance` anyways.

For `HasLowerBound` to be true the instantiation of `std::lower_bound` has to be valid. So if `HasLowerBound` is true `int main() { std::lower_bound(iter, sent, 0); }` has to compile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127577