Page MenuHomePhabricator

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

Authored by thomasanderson on Apr 11 2019, 5:22 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

thomasanderson created this revision.Apr 11 2019, 5:22 PM

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 */
thomasanderson marked 4 inline comments as done.
EricWF accepted this revision.Apr 13 2019, 10:22 AM
This revision is now accepted and ready to land.Apr 13 2019, 10:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 10:02 AM

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

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.

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.

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 !