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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
