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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

This LGTM, as a temporary fix;  once we test it.

I'm going to request further changes, but this is a OK intermediate state, so we don't have to address them before landing this.
Thank you for you diligent work fixing this on the weekend.

______________________________________________

Given the mess with C++03 and strong enumerations types, I'm believe we should move to using a policy object like

  struct RangesIterOps {
    static It advance(...);
    static diff_t distance(...);
  };
  struct StdIterOps {
    static It advance(...);
    static diff_t distance(...);
  };

I think it has the added benefits of:

- Making it clear exactly what set of functions are overloaded for std iterators and ranges iterators. (it's the set of functions provided by the policy object, rather than a subset of functions discoverable only by reading every single iterator/ranges headers to see which are overloaded).
- Removing the need for almost every single compile time branch (both `#ifdef` and `if constexpr`.) currently used by `__ranges_distance` and `__ranges_advance`. And every single compile time branch in libc++ is a bug manifest, or at least a future bug that will manifest; at least that's been my experience).
- It removes the need for the `__ranges_foo` overloads entirely. Because it's not obvious to the reader that `__distance` is `std::distance`, and `__ranges_distance` is a dispatcher that may call `ranges::distance` or `std::distance`. In fact, `__ranges_distance` seems easily mistakable for the `ranges::distance` implementation.
- It further isolates us from differences in behavior, for example like `iterator_traits<_Iter>::difference_type` being different than `std::iter_distance_t<Iter>` and similar.



================
Comment at: libcxx/include/__algorithm/generic_algorithm_helper.h:9
+
+#ifndef _LIBCPP___ALGORIHTM_GENERIC_ALGORIHTM_HELPER_H
+#define _LIBCPP___ALGORIHTM_GENERIC_ALGORIHTM_HELPER_H
----------------
I'm going to complain about the header name again.

One of the primary reasons I've been given for splitting up the headers is that "it makes it easier to know where an implementation actually lives", but "generic_algorithm_helper.h" doesn't indicate to me that it contains the `_Namespace` utility.


================
Comment at: libcxx/include/__algorithm/generic_algorithm_helper.h:21
+namespace _Namespace {
+enum _Namespace : bool {
+  _Ranges,
----------------
Or `struct _Namespace { enum : bool { _Std, _Ranges }; };`. Having the symbol have two entirely different meanings has a mean code smell.


================
Comment at: libcxx/include/__iterator/distance.h:108
 _LIBCPP_CONSTEXPR_AFTER_CXX11
 typename iterator_traits<_Iter>::difference_type __ranges_distance(_Iter __first, _Sent __second) {
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
----------------
Is it the case that `iterator_traits<_Iter>::difference_type` is always the same type as `std::iter_distance_t<_Iter>`? It's not obvious that it is a requirement.


================
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) {
----------------
philnik wrote:
> 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)`.
Ah, that makes sense.

This further pushes me towards the idea that a `IteratorOperations` policy object is the right way to go.


================
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); };
----------------
philnik wrote:
> 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.
That's not correct. Concepts only consider the validity of the call expression, not everything that would be instantiated. (Because it can't back of of an instantiation once it's started instantiating, since there could be side effects and the like). So it doesn't consider the body of lower bound.

This test passes without your fix. See https://godbolt.org/z/5hfv8njeo

So we need a test that actually runs and produces an output to be sure we've fully instantiated everything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127577