Page MenuHomePhabricator

Fixing all comparisons for C++20 compilation.
AcceptedPublic

Authored by BRevzin on Apr 27 2020, 9:14 AM.

Details

Reviewers
jfb
jdoerfert
Summary

Part of the <=> changes in C++20 make certain patterns of writing equality operators ambiguous with themselves (sorry!). This review goes through and adjusts all the comparison oeprators such that they should work in both C++17 and C++20 modes.

There were four categories of errors that this review fixes. Here are canonical examples of them, ordered from most to least common, which you can view on compiler-explorer:

#include <utility>

// 1) Missing const
namespace missing_const {
    struct A {
    #ifndef FIXED
        bool operator==(A const&);
    #else
        bool operator==(A const&) const;
    #endif
    };

    bool a = A{} == A{}; // error
}

// 2) Type mismatch on CRTP
namespace crtp_mismatch {
    template <typename Derived>
    struct Base {
    #ifndef FIXED
        bool operator==(Derived const&) const;
    #else
        // in one case changed to taking Base const&
        friend bool operator==(Derived const&, Derived const&);
    #endif
    };

    struct D : Base<D> { };

    bool b = D{} == D{}; // error
}

// 3) iterator/const_iterator with only mixed comparison
namespace iter_const_iter {
    template <bool Const>
    struct iterator {
        using const_iterator = iterator<true>;

        iterator();

        template <bool B, std::enable_if_t<(Const && !B), int> = 0>
        iterator(iterator<B> const&);

    #ifndef FIXED
        bool operator==(const_iterator const&) const;
    #else
        friend bool operator==(iterator const&, iterator const&);
    #endif
    };
    
    bool c = iterator<false>{} == iterator<false>{} // error
          || iterator<false>{} == iterator<true>{}
          || iterator<true>{} == iterator<false>{}
          || iterator<true>{} == iterator<true>{};
}

// 4) Same-type comparison but only have mixed-type operator
namespace ambiguous_choice {
    enum Color { Red };

    struct C {
        C();
        C(Color);
        operator Color() const;
        bool operator==(Color) const;
#ifdef FIXED
        friend bool operator==(C, C);
#endif
    };

    bool c = C{} == C{}; // error
    bool d = C{} == Red;
}

Diff Detail

Event Timeline

BRevzin created this revision.Apr 27 2020, 9:14 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
llvm/include/llvm/Support/BinaryStreamRef.h
124–125

Is there a neat rule of thumb for when you were able to preserve the member bool Me::operator==(const Me& rhs) const versus when you were forced to change it to a hidden friend? It seems like maybe you changed it to a hidden friend only in cases where Me was a base class, is that right?

rsmith added a subscriber: rsmith.Apr 27 2020, 12:06 PM
rsmith added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
64–67

Comparing against V.Value here would seem more idiomatic than invoking operator unsigned.

69–70

Do we still need these?

BRevzin edited the summary of this revision. (Show Details)Apr 27 2020, 3:17 PM
BRevzin updated this revision to Diff 260477.Apr 27 2020, 3:29 PM

More idiomatic comparison implementation.

BRevzin marked 2 inline comments as done.Apr 27 2020, 3:30 PM
BRevzin added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
69–70

Yep. This type is compared to both itself and the enum, so both are necessary.

Wtf, where'd my other changes go?

Wtf, where'd my other changes go?

I've hit this before, use arc diff --update D78938 <base_branch>.

BRevzin updated this revision to Diff 260521.Apr 27 2020, 6:17 PM

Trying this again.

Wtf, where'd my other changes go?

I've hit this before, use arc diff --update D78938 <base_branch>.

Thanks Hubert!

(peanut gallery: I'd consider, while you're touching these all anyway, changing them all to non-member (friended where required) as I believe that's best practice - allows equal implicit conversions on either side, for instance (even if some types have no implicit conversions - it at least provides a nice consistency/examples that people are likely to copy from))

(peanut gallery: I'd consider, while you're touching these all anyway, changing them all to non-member (friended where required) as I believe that's best practice - allows equal implicit conversions on either side, for instance (even if some types have no implicit conversions - it at least provides a nice consistency/examples that people are likely to copy from))

Hidden friend is probably the best way to write comparisons in C++17 and earlier, but I'm not sure that will hold in C++20 (even if LLVM isn't on C++20 and won't be for I imagine quite some time). With reversed candidates, I think member functions might be the way to go there - you still get implicit conversions on either side (just not on both sides at the same time) and hidden friends... are kind of weird, to be honest.

Also, I didn't touch all of them - only the ones that break in C++20 (a lot of which just missing a const). A lot of comparison operators are already fine. I'm not sure it's worth changing them just to look the same.

(peanut gallery: I'd consider, while you're touching these all anyway, changing them all to non-member (friended where required) as I believe that's best practice - allows equal implicit conversions on either side, for instance (even if some types have no implicit conversions - it at least provides a nice consistency/examples that people are likely to copy from))

Hidden friend is probably the best way to write comparisons in C++17 and earlier, but I'm not sure that will hold in C++20 (even if LLVM isn't on C++20 and won't be for I imagine quite some time). With reversed candidates, I think member functions might be the way to go there - you still get implicit conversions on either side (just not on both sides at the same time) and hidden friends... are kind of weird, to be honest.

Yeah, probably just experience with things the way they've been, but the symmetry is kind of nice without relying on deeper aspects of the newer features (& the benefit of the code being more suitable for C++17, where LLVM is currently).

Also, I didn't touch all of them - only the ones that break in C++20 (a lot of which just missing a const). A lot of comparison operators are already fine. I'm not sure it's worth changing them just to look the same.

Yeah - just meant the ones you are touching, might be nice to move them in that direction.

Anyway, I'll leave it to you/other reviewers - no /super/ strong feelings here.

jdoerfert resigned from this revision.Apr 30 2020, 6:08 AM
jfb accepted this revision.May 18 2020, 8:28 AM
jfb added a subscriber: jfb.

This seems fine, assuming you've run usual tests?

This revision is now accepted and ready to land.May 18 2020, 8:28 AM
davidstone added inline comments.
llvm/include/llvm/ADT/DirectedGraph.h
99

Missing return

BRevzin updated this revision to Diff 265874.May 23 2020, 10:20 AM
  • A few more changes from tests.
BRevzin updated this revision to Diff 265875.May 23 2020, 10:25 AM
  • Adding missing return.
BRevzin marked 2 inline comments as done.May 23 2020, 10:27 AM

I hadn't build the tests before, updated with a few more changes. Some of the tests require u8 literals, whose type changes in C++20. I had no idea what to do with that, so I just #ifdef-ed out those tests with the appropriate feature test macro.

BRevzin updated this revision to Diff 265900.May 23 2020, 5:11 PM
  • Backing out changes that aren't strictly comparison-related.
jfb accepted this revision.May 24 2020, 2:56 PM

One suggestions, otherwise looks good. Thanks for doing this :)

llvm/include/llvm/ADT/DirectedGraph.h
40–41

That comment, so informative! 😐

99

😱

Did this not trigger a diagnostic when building? I wonder if it's just not on?

llvm/unittests/ADT/STLExtrasTest.cpp
467

Can you add a comment above (with "fancy pointer") so mere mortals understand the parens?

I noticed the missing return because there is a warning (not as error) that caught it, I think the warning about falling off the end of a non-void-returning function.

BRevzin updated this revision to Diff 266071.May 25 2020, 1:27 PM
BRevzin marked 2 inline comments as done.
  • Explaining the cryptic parentheses.
llvm/include/llvm/ADT/DirectedGraph.h
99

Yeah I was surprised too. I'm compiling with -Wall -Wextra...