[PATCH] D78938: Make LLVM build in C++20 mode

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D78938#2262713 <https://reviews.llvm.org/D78938#2262713>, @jhenderson wrote:

> In D78938#2261411 <https://reviews.llvm.org/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.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:167-171
-
-inline bool operator!=(const DWARFExpression::iterator &LHS,
-                       const DWARFExpression::iterator &RHS) {
-  return !(LHS == RHS);
-}
----------------
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)


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:324-325
+    template <typename PHINodeU, typename BBIteratorU,
+              typename = std::enable_if_t<
+                  std::is_convertible<PHINodeU *, PHINodeT *>::value>>
     phi_iterator_impl(const phi_iterator_impl<PHINodeU, BBIteratorU> &Arg)
----------------
What tripped over/required this SFINAE?


================
Comment at: llvm/include/llvm/Support/BinaryStreamRef.h:124
 
-  bool operator==(const RefType &Other) const {
-    if (BorrowedImpl != Other.BorrowedImpl)
+  friend bool operator==(const RefType &Self, const RefType &Other) {
+    if (Self.BorrowedImpl != Other.BorrowedImpl)
----------------
Quuxplusone wrote:
> 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?
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)


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:465
   // Check fancy pointer overload for unique_ptr
+  // Parenthesizing to_address to avoid ADL finding std::to_address
   std::unique_ptr<int> V2 = std::make_unique<int>(0);
----------------
jhenderson wrote:
> Nit: trailing full stop.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938