[libcxx-commits] [PATCH] D60592: [libc++] Fix build failure with _LIBCPP_DEBUG=0 when iterators return values instead of references

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

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.



================
Comment at: libcxx/include/algorithm:807
+    decltype((void)_VSTD::declval<_Compare&>()(
+        _VSTD::declval<const _LHS &>(), _VSTD::declval<const _RHS &>()))
+    __do_compare_assert(int, const _LHS & __l, const _RHS & __r) {
----------------
This change shouldn't be needed. We always call `__do_compare_assert` with an lvalue, so the additional const overloads aren't needed.


================
Comment at: libcxx/include/algorithm:824
+    inline _LIBCPP_INLINE_VISIBILITY
+    void __do_compare_assert(long, const _LHS &, const _RHS &) {}
+
----------------
This should be unneeded either.


================
Comment at: libcxx/test/libcxx/algorithms/debug_less.pass.cpp:242
+    using difference_type = ptrdiff_t;
+    using reference = size_t&;
+    using pointer = size_t*;
----------------
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`


================
Comment at: libcxx/test/libcxx/algorithms/debug_less.pass.cpp:259
+}
+
 int main(int, char**) {
----------------
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 */
```


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D60592