[libc++] Fix build failure with _LIBCPP_DEBUG=0 when iterators return values instead of references

[thomasanderson]

Summary

There are many STL algorithms (such as lexicographical_compare) that compare
values pointed to by iterators like so:

    __comp(*it1, *it2);

When building with _LIBCPP_DEBUG=0, comparators are wrapped in __debug_less
which does some additional validation. But __debug_less::operator() takes
non-const references, so if the type of *it1 is int, not int&, then the build
will fail.

This change adds a const& overload for operator() to fix the build.

[thomasanderson]

@EricWF ptal. I think this regressed after https://reviews.llvm.org/rCXX355752

[EricWF]

Hmm. I thought I had fixed this using perfect forwarding, but I guess not. My bad.

This patch looks like a correct approach to me.

libcxx/include/algorithm

807 ↗

(On Diff #194788)

This change shouldn't be needed. We always call __do_compare_assert with an lvalue, so the additional const overloads aren't needed.

824 ↗

(On Diff #194788)

This should be unneeded either.

libcxx/test/libcxx/algorithms/debug_less.pass.cpp

242 ↗

(On Diff #194788)

Only input iterators can have a reference typedef that isn't actually a reference.

This test should use an input iterator instead, and the reference typedef should be the same as value_type, and the operator*() should spell the return type reference

259 ↗

(On Diff #194788)

It would be nice to have another test that created a __debug_less<Comp> and directly called it with arguments of various value categories.

std::less<int> l;
std::__debug_less<std::less<int>> dl(l);
int lvalue = 42;
const int const_lvalue = 101;
assert(d(lvalue, const_lvalue));
assert(dl(/*rvalue*/1, lvalue));
/* etc */

[ldionne]

This seems to have broken some CI because we're using aliases instead of typedefs and this file needs to work in C++03.

[ldionne]

In D60592#1467243, @ldionne wrote:

This seems to have broken some CI because we're using aliases instead of typedefs and this file needs to work in C++03.

I'm working on a fix.

[ldionne]

In D60592#1467247, @ldionne wrote:

In D60592#1467243, @ldionne wrote:

This seems to have broken some CI because we're using aliases instead of typedefs and this file needs to work in C++03.

I'm working on a fix.

Should be fixed in r358433.

[thomasanderson]

In D60592#1467248, @ldionne wrote:

In D60592#1467247, @ldionne wrote:

In D60592#1467243, @ldionne wrote:

This seems to have broken some CI because we're using aliases instead of typedefs and this file needs to work in C++03.

I'm working on a fix.

Should be fixed in r358433.

Thank you @ldionne !

libcxx/trunk/include/algorithm

 template <class _Compare>
 struct __debug_less
 {
     _Compare &__comp_;
     _LIBCPP_CONSTEXPR_AFTER_CXX17
     __debug_less(_Compare& __c) : __comp_(__c) {}
 
     template <class _Tp, class _Up>
+    bool operator()(const _Tp& __x,  const _Up& __y)
+    {
+        bool __r = __comp_(__x, __y);
+        if (__r)
+            __do_compare_assert(0, __y, __x);
+        return __r;
+    }
+
+    template <class _Tp, class _Up>
     _LIBCPP_CONSTEXPR_AFTER_CXX17
     bool operator()(_Tp& __x,  _Up& __y)
     {
         bool __r = __comp_(__x, __y);
         if (__r)
             __do_compare_assert(0, __y, __x);
         return __r;
     }

libcxx/trunk/test/libcxx/algorithms/debug_less.pass.cpp

void test_non_const_arg_cmp() {
     {
         NonConstArgCmp cmp;
         int arr[] = {5, 4, 3, 2, 1};
         std::sort(std::begin(arr), std::end(arr), cmp);
         assert(std::is_sorted(std::begin(arr), std::end(arr)));
     }
 }
 
+struct ValueIterator {
+    using iterator_category = std::input_iterator_tag;
+    using value_type = size_t;
+    using difference_type = ptrdiff_t;
+    using reference = size_t;
+    using pointer = size_t*;
+
+    ValueIterator() = default;
+
+    reference operator*() { return 0; }
+    ValueIterator& operator++() { return *this; }
+
+    friend bool operator==(ValueIterator, ValueIterator) { return true; }
+    friend bool operator!=(ValueIterator, ValueIterator) { return false; }
+};
+
+void test_value_iterator() {
+    // Ensure no build failures when iterators return values, not references.
+    assert(0 == std::lexicographical_compare(ValueIterator{}, ValueIterator{},
+                                             ValueIterator{}, ValueIterator{}));
+}
+
+void test_value_categories() {
+    std::less<int> l;
+    std::__debug_less<std::less<int>> dl(l);
+    int lvalue = 42;
+    const int const_lvalue = 101;
+
+    assert(dl(lvalue, const_lvalue));
+    assert(dl(/*rvalue*/1, lvalue));
+    assert(dl(static_cast<int&&>(1), static_cast<const int&&>(2)));
+}
+
 int main(int, char**) {
     test_passing();
     test_failing();
     test_upper_and_lower_bound();
     test_non_const_arg_cmp();
+    test_value_iterator();
+    test_value_categories();
     return 0;
 }