[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.

Is this missing module map changes?

This needs a test that fails prior to the fix. Otherwise this looks good once the inline comments are addressed.



================
Comment at: libcxx/include/__algorithm/generic_algorithm_helper.h:18
+
+enum _Namespace {
+  _Ranges,
----------------
* Inside namespace `std` please.
* Scoped enum please. Implicit conversions are evil.
* Pin the underlying type to prevent `-fshort-enums` from changing it.



================
Comment at: libcxx/include/__iterator/advance.h:200
 _LIBCPP_CONSTEXPR_AFTER_CXX11 void __ranges_advance(_Iter& __first, _Sent& __last) {
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
+  if constexpr (__ns == _Namespace::_Ranges) {
----------------
Same comment about this `#ifdef` as I left on `__distance`.


================
Comment at: libcxx/include/__iterator/distance.h:13
 
+#include <__algorithm/generic_algorithm_helper.h>
 #include <__config>
----------------
I would love a more descriptive name for this header.


================
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) {
----------------
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::`


================
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); };
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127577