Page MenuHomePhabricator

Make LLVM build in C++20 mode
AcceptedPublic

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

Details

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 operators such that they should work in both C++17 and C++20 modes. It also makes two other small C++20-specific changes (adding a constructor to a type that cases to be an aggregate, and adding casts from u8 literals which no longer have type const char*)

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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...

BRevzin updated this revision to Diff 290023.Sep 4 2020, 2:01 PM

Updating this review with some additional changes that need to be made since I last touched it, and some of the previous changes had inadvertently broken the C++14 build so fixing those as well.

Not that I have anything particularly against this, but won't this likely rot fairly rapidly? It's not like LLVM is even on C++17 let alone C++20 yet, so trying to make it work like the latter when it's just going to break again seems a bit like wasted effort to me.

llvm/tools/llvm-objdump/llvm-objdump.cpp
805–817

This seems unrelated to comparison checking?

llvm/unittests/ADT/STLExtrasTest.cpp
465

Nit: trailing full stop.

Not that I have anything particularly against this, but won't this likely rot fairly rapidly? It's not like LLVM is even on C++17 let alone C++20 yet, so trying to make it work like the latter when it's just going to break again seems a bit like wasted effort to me.

People will want to write C++20 programs that use LLVM headers, so I think it's important to help let them do that. Sure, it may rot, but incremental fixes down the line will be smaller.

BRevzin added inline comments.Sep 7 2020, 8:17 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
805–817

This seems unrelated to comparison checking?

It is unrelated. But In C++20, u8 literals become their own type so this no longer compiled and I wanted to ensure that I could actually run the tests.

Not that I have anything particularly against this, but won't this likely rot fairly rapidly? It's not like LLVM is even on C++17 let alone C++20 yet, so trying to make it work like the latter when it's just going to break again seems a bit like wasted effort to me.

People will want to write C++20 programs that use LLVM headers, so I think it's important to help let them do that. Sure, it may rot, but incremental fixes down the line will be smaller.

Makes sense, thanks.

llvm/tools/llvm-objdump/llvm-objdump.cpp
805–817

Could it be a pre-requisite patch then?

martong removed a subscriber: martong.Sep 8 2020, 6:23 AM
jfb added a comment.Sep 8 2020, 10:26 AM

On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch.

llvm/tools/llvm-objdump/llvm-objdump.cpp
805–817

I'm fine with this if the patch title is changed to "make LLVM build in C++20 mode", and description edited accordingly. Basically, it makes it easy to figure out which changes were done for C++20.

BRevzin retitled this revision from Fixing all comparisons for C++20 compilation. to Make LLVM build in C++20 mode.Sep 8 2020, 11:17 AM
BRevzin edited the summary of this revision. (Show Details)
In D78938#2261411, @jfb wrote:

On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch.

I assume this would be a private bot? It can't be a public bot, since LLVM isn't even on C++17, let alone C++20, and so it shouldn't be part of minimum requirements that somebody has a compiler that can build C++20. Whilst I personally am quite happy with moving LLVM forward, I develop on Windows primarily, so don't have the same need to support a long tail of old *nix versions etc.

@BRevzin, you should a) mention the u8/const char* issue in the description too, and also what compiler you used to build this with. I fully expect at this stage that there are some C++20 compilers that might have slightly different interpretations of things which this won't resolve, so knowing which one this is intended to work with could help with historical research.

@BRevzin, you should a) mention the u8/const char* issue in the description too, and also what compiler you used to build this with. I fully expect at this stage that there are some C++20 compilers that might have slightly different interpretations of things which this won't resolve, so knowing which one this is intended to work with could help with historical research.

It's mentioned in the description. I built with clang-10.

In D78938#2261411, @jfb wrote:

On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch.

I assume this would be a private bot? It can't be a public bot, since LLVM isn't even on C++17, let alone C++20, and so it shouldn't be part of minimum requirements that somebody has a compiler that can build C++20. Whilst I personally am quite happy with moving LLVM forward, I develop on Windows primarily, so don't have the same need to support a long tail of old *nix versions etc.

I'd be fine with it being a public bot - it's not saying LLVM can only be compiled with C++20-supporting compilers, that'd be very different & that's the discussion we'll have when we want to start using C++20 in LLVM. But saying "LLVM is intended to be C++20 compatible" is something we can/shuold be saying much sooner than that. Like we say that LLVM's compatible with a certain variety of compilers in C++14 mode too - not everyone has or is testing on all those compilers every time they commit, but buildbots test a range of them (and test a range of hardware - again, hardware I don't have/don't intend to test with) & we clean things up that they report, ideally.

I'd think a C++20 buildbot could at least be relatively fast/easy (doesn't have to do multi-stage bootstraps (though it could - making sure the evolving C++20 support in Clang remains compatible with the LLVM project codebase itself)) - doesn't even need to run any tests, really, just compile.

llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
167–171

Why are some being removed? That seems harder to justify. Even if they're not called, it may be more valuable to have the symmetry to reduce friction if/when they are needed. (iterators seem pretty common to compare for inequality - such as in a loop condition testing I != E)

llvm/include/llvm/IR/BasicBlock.h
324–325

What tripped over/required this SFINAE?

llvm/include/llvm/Support/BinaryStreamRef.h
124–125

Be curious of the answer here - and, honestly, I'd be fine with changing them all to friends. It makes them more consistent - equal rank for implicit conversions on LHS and RHS, etc. (generally considered best practice basically to not define op overloads as members if they can be defined as non-members)

llvm/unittests/ADT/STLExtrasTest.cpp
465

Probably more suitable to use qualify the name rather than use parens (teh comment's still helpful to explain why either strategy is used) - that's what's done with llvm::make_unique, for instance.

BRevzin added inline comments.Sep 27 2020, 7:56 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
167–171

They're not being removed. These functions still exist - it's just that now they're being injected by the base class template with this exact signature (rather than before where they were slightly different), so that now these are redefinition issues.

There's no loss of functionality here.

llvm/include/llvm/IR/BasicBlock.h
324–325

There's somewhere which compared a const iterator to a non-const iterator, that ends up doing conversions in both directions under C++20 rules, one direction of which is perfectly fine and the other was a hard error. Need to make the non-const iterator not constructible from a const iterator.

dblaikie added inline comments.Sep 28 2020, 1:59 PM
llvm/include/llvm/IR/BasicBlock.h
324–325

Is this true for all iterators? Or some quirk of how this one is written/used (that could be fixed/changed there instead)?

Quuxplusone added inline comments.Sep 28 2020, 4:25 PM
llvm/include/llvm/IR/BasicBlock.h
324–325

IMO there is a (much) bigger task hiding here, which is to audit every type in the codebase whose name contains the string "Iterator" and compare them to the C++20 Ranges std::forward_iterator concept. My impression is that the vast majority of real-world "iterator types" are not iterators according to C++20 Ranges, and that this can have arbitrarily weird effects when you mix them with the C++20 STL.

However, that is massive scope creep re this particular patch. I think the larger question of "do all our iterators need X / are all our iterators written wrong" should be scoped-outside-of this patch.

dblaikie added inline comments.Sep 28 2020, 4:40 PM
llvm/include/llvm/IR/BasicBlock.h
324–325

Sorry, not suggesting that kind of scope creep - but do want to understand whether this is representative of the way code should generally be written, or whether this is working around some other issue/different fix.

BRevzin added inline comments.Sep 29 2020, 7:08 AM
llvm/include/llvm/IR/BasicBlock.h
324–325

So I undid this change to copy the exact issue that I ran into. But it actually ended up still compiling anyway. Part of the issue might be that I keep futzing with the cmake configuration since it takes me more than an hour to compile, so maybe there's some target that needed this change that I no longer compile.

But the kind of problem I think this had was:

template <typename T>
struct iterator {
    T* p;
    
    template <typename U>
    iterator(iterator<U> rhs)
        : p(rhs.p)
    { } 

    bool operator==(iterator const& rhs);
};

bool check(iterator<int const> a, iterator<int> b) {
    return a == b;
}

which compiles fine in C++17 but is ambiguous in C++20 because b.operator==(a) is also a candidate (even though it's not _really_ a candidate, and would be a hard error if selected). the sfinae removes the bad candidate from the set.

It's true for all iterators in general in that you want const_iterator constructible from iterator but not the reverse (unless they're the same type).

lebedev.ri added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
167–171

Does LLVM still build fine in C++14/C++17 modes afterwards?

BRevzin added inline comments.Sep 29 2020, 7:19 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
167–171

Yes.

dblaikie added inline comments.Sep 29 2020, 8:45 AM
llvm/include/llvm/IR/BasicBlock.h
324–325

Fair enough - don't mind keeping it in then.

llvm/include/llvm/Support/BinaryStreamRef.h
124–125

Ping on this (& I'd usually call the parameters LHS and RHS rather than Self and Other)

llvm/unittests/ADT/STLExtrasTest.cpp
465

Ping on this.