Remove redundant LLVM_HAS_RVALUE_REFERENCE_THIS and LLVM_LVALUE_FUNCTION defines

[RKSimon]

Summary

Now that VS2017 support has been dropped (D114639), the LLVM_HAS_RVALUE_REFERENCE_THIS define is always true and the LLVM_LVALUE_FUNCTION define is always enabled for ref-qualifiers.

This patch proposes we remove the defines and use the qualifiers directly.

NOTE: I'm not intending to push this before the 14.x branch.

[aaron.ballman]

LGTM! I had tried doing this once before and had to revert due to MSVC miscompiles, so be sure to watch for fallout (I appreciate you not landing before the branch because of this). Can you fix the clang-format issues before landing?

[RKSimon]

clang-format

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

public:
   Matcher(const Matcher<TypeT> &Other,
           std::enable_if_t<std::is_same<T, QualType>::value &&
                            std::is_same<TypeT, Type>::value> * = nullptr)
       : Implementation(new TypeToQualType<TypeT>(Other)) {}
 
   /// Convert \c this into a \c Matcher<T> by applying dyn_cast<> to the
   /// argument.
   /// \c To must be a base class of \c T.
-  template <typename To> Matcher<To> dynCastTo() const LLVM_LVALUE_FUNCTION {
+  template <typename To> Matcher<To> dynCastTo() const & {
     static_assert(std::is_base_of<To, T>::value, "Invalid dynCast call.");
     return Matcher<To>(Implementation);
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   template <typename To> Matcher<To> dynCastTo() && {
     static_assert(std::is_base_of<To, T>::value, "Invalid dynCast call.");
     return Matcher<To>(std::move(Implementation));
   }
-#endif
 
   /// Forwards the call to the underlying MatcherInterface<T> pointer.
   bool matches(const T &Node,
                ASTMatchFinder *Finder,
                BoundNodesTreeBuilder *Builder) const {
     return Implementation.matches(DynTypedNode::create(Node), Finder, Builder);
   }
 
   /// Returns an ID that uniquely identifies the matcher.
   DynTypedMatcher::MatcherIDType getID() const {
     return Implementation.getID();
   }
 
   /// Extract the dynamic matcher.
   ///
   /// The returned matcher keeps the same restrictions as \c this and remembers
   /// that it is meant to support nodes of type \c T.
-  operator DynTypedMatcher() const LLVM_LVALUE_FUNCTION {
-    return Implementation;
-  }
+  operator DynTypedMatcher() const & { return Implementation; }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   operator DynTypedMatcher() && { return std::move(Implementation); }
-#endif
 
   /// Allows the conversion of a \c Matcher<Type> to a \c
   /// Matcher<QualType>.
   ///
   /// Depending on the constructor argument, the matcher is either strict, i.e.
   /// does only matches in the absence of qualifiers, or not, i.e. simply
   /// ignores any qualifiers.
   template <typename TypeT>

 /// Input matchers can have any type (including other polymorphic matcher
 /// types), and the actual Matcher<T> is generated on demand with an implicit
 /// conversion operator.
 template <typename... Ps> class VariadicOperatorMatcher {
 public:
   VariadicOperatorMatcher(DynTypedMatcher::VariadicOperator Op, Ps &&... Params)
       : Op(Op), Params(std::forward<Ps>(Params)...) {}
 
-  template <typename T> operator Matcher<T>() const LLVM_LVALUE_FUNCTION {
+  template <typename T> operator Matcher<T>() const & {
     return DynTypedMatcher::constructVariadic(
                Op, ASTNodeKind::getFromNodeKind<T>(),
                getMatchers<T>(std::index_sequence_for<Ps...>()))
         .template unconditionalConvertTo<T>();
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   template <typename T> operator Matcher<T>() && {
     return DynTypedMatcher::constructVariadic(
                Op, ASTNodeKind::getFromNodeKind<T>(),
                getMatchers<T>(std::index_sequence_for<Ps...>()))
         .template unconditionalConvertTo<T>();
   }
-#endif
+
 private:
   // Helper method to unpack the tuple into a vector.
   template <typename T, std::size_t... Is>
-  std::vector<DynTypedMatcher>
-  getMatchers(std::index_sequence<Is...>) const LLVM_LVALUE_FUNCTION {
+  std::vector<DynTypedMatcher> getMatchers(std::index_sequence<Is...>) const & {
     return {Matcher<T>(std::get<Is>(Params))...};
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   template <typename T, std::size_t... Is>
   std::vector<DynTypedMatcher> getMatchers(std::index_sequence<Is...>) && {
     return {Matcher<T>(std::get<Is>(std::move(Params)))...};
   }
-#endif
 
   const DynTypedMatcher::VariadicOperator Op;
   std::tuple<Ps...> Params;
 };
 
 /// Overloaded function object to generate VariadicOperatorMatcher
 ///   objects from arbitrary matchers.
 template <unsigned MinCount, unsigned MaxCount>
template <template <typename ToArg, typename FromArg> class ArgumentAdapterT,
           typename T, typename ToTypes>
 class ArgumentAdaptingMatcherFuncAdaptor {
 public:
   explicit ArgumentAdaptingMatcherFuncAdaptor(const Matcher<T> &InnerMatcher)
       : InnerMatcher(InnerMatcher) {}
 
   using ReturnTypes = ToTypes;
 
-  template <typename To> operator Matcher<To>() const LLVM_LVALUE_FUNCTION {
+  template <typename To> operator Matcher<To>() const & {
     return Matcher<To>(new ArgumentAdapterT<To, T>(InnerMatcher));
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   template <typename To> operator Matcher<To>() && {
     return Matcher<To>(new ArgumentAdapterT<To, T>(std::move(InnerMatcher)));
   }
-#endif
 
 private:
   Matcher<T> InnerMatcher;
 };
 
 /// Converts a \c Matcher<T> to a matcher of desired type \c To by
 /// "adapting" a \c To into a \c T.
 ///
public:
   }
 };
 
 template <typename MatcherType> class TraversalWrapper {
 public:
   TraversalWrapper(TraversalKind TK, const MatcherType &InnerMatcher)
       : TK(TK), InnerMatcher(InnerMatcher) {}
 
-  template <typename T> operator Matcher<T>() const LLVM_LVALUE_FUNCTION {
+  template <typename T> operator Matcher<T>() const & {
     return internal::DynTypedMatcher::constructRestrictedWrapper(
                new internal::TraversalMatcher<T>(TK, InnerMatcher),
                ASTNodeKind::getFromNodeKind<T>())
         .template unconditionalConvertTo<T>();
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   template <typename T> operator Matcher<T>() && {
     return internal::DynTypedMatcher::constructRestrictedWrapper(
                new internal::TraversalMatcher<T>(TK, std::move(InnerMatcher)),
                ASTNodeKind::getFromNodeKind<T>())
         .template unconditionalConvertTo<T>();
   }
-#endif
 
 private:
   TraversalKind TK;
   MatcherType InnerMatcher;
 };
 
 /// A PolymorphicMatcher<MatcherT, P1, ..., PN> object can be
 /// created from N parameters p1, ..., pN (of type P1, ..., PN) and

 template <template <typename T, typename... Params> class MatcherT,
           typename ReturnTypesF, typename... ParamTypes>
 class PolymorphicMatcher {
 public:
   PolymorphicMatcher(const ParamTypes &... Params) : Params(Params...) {}
 
   using ReturnTypes = typename ExtractFunctionArgMeta<ReturnTypesF>::type;
 
-  template <typename T> operator Matcher<T>() const LLVM_LVALUE_FUNCTION {
+  template <typename T> operator Matcher<T>() const & {
     static_assert(TypeListContainsSuperOf<ReturnTypes, T>::value,
                   "right polymorphic conversion");
     return Matcher<T>(new_from_tuple<MatcherT<T, ParamTypes...>>(Params));
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   template <typename T> operator Matcher<T>() && {
     static_assert(TypeListContainsSuperOf<ReturnTypes, T>::value,
                   "right polymorphic conversion");
     return Matcher<T>(
         new_from_tuple<MatcherT<T, ParamTypes...>>(std::move(Params)));
   }
-#endif
 
 private:
   std::tuple<ParamTypes...> Params;
 };
 
 /// Matches nodes of type T that have child nodes of type ChildT for
 /// which a specified child matcher matches.
 ///

clang/include/clang/Basic/DirectoryEntry.h

public:
   template <class... ArgTypes>
   explicit MapEntryOptionalStorage(llvm::in_place_t, ArgTypes &&...Args)
       : MaybeRef(std::forward<ArgTypes>(Args)...) {}
 
   void reset() { MaybeRef = optional_none_tag(); }
 
   bool hasValue() const { return MaybeRef.hasOptionalValue(); }
 
-  RefTy &getValue() LLVM_LVALUE_FUNCTION {
+  RefTy &getValue() & {
     assert(hasValue());
     return MaybeRef;
   }
-  RefTy const &getValue() const LLVM_LVALUE_FUNCTION {
+  RefTy const &getValue() const & {
     assert(hasValue());
     return MaybeRef;
   }
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   RefTy &&getValue() && {
     assert(hasValue());
     return std::move(MaybeRef);
   }
-#endif
 
   template <class... Args> void emplace(Args &&...args) {
     MaybeRef = RefTy(std::forward<Args>(args)...);
   }
 
   MapEntryOptionalStorage &operator=(RefTy Ref) {
     MaybeRef = Ref;
     return *this;

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h

public:
   CFG &getCFG() const { return *getLocationContext()->getCFG(); }
 
   const CFGBlock *getCFGBlock() const;
 
   const ParentMap &getParentMap() const {
     return getLocationContext()->getParentMap();
   }
 
-  template <typename T>
-  T &getAnalysis() const {
+  template <typename T> T &getAnalysis() const {
     return *getLocationContext()->getAnalysis<T>();
   }
 
   const ProgramStateRef &getState() const { return State; }
 
-  template <typename T>
-  Optional<T> getLocationAs() const LLVM_LVALUE_FUNCTION {
+  template <typename T> Optional<T> getLocationAs() const & {
     return Location.getAs<T>();
   }
 
   /// Get the value of an arbitrary expression at this node.
   SVal getSVal(const Stmt *S) const {
     return getState()->getSVal(S, getLocationContext());
   }
 

llvm/include/llvm/ADT/Optional.h

public:
   }
   constexpr OptionalStorage(OptionalStorage &&other) : OptionalStorage() {
     if (other.hasValue()) {
       emplace(std::move(other.value));
     }
   }
 
   template <class... Args>
-  constexpr explicit OptionalStorage(in_place_t, Args &&... args)
+  constexpr explicit OptionalStorage(in_place_t, Args &&...args)
       : value(std::forward<Args>(args)...), hasVal(true) {}
 
   void reset() noexcept {
     if (hasVal) {
       value.~T();
       hasVal = false;
     }
   }
 
   constexpr bool hasValue() const noexcept { return hasVal; }
 
-  T &getValue() LLVM_LVALUE_FUNCTION noexcept {
+  T &getValue() &noexcept {
     assert(hasVal);
     return value;
   }
-  constexpr T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
+  constexpr T const &getValue() const &noexcept {
     assert(hasVal);
     return value;
   }
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
-  T &&getValue() && noexcept {
+  T &&getValue() &&noexcept {
     assert(hasVal);
     return std::move(value);
   }
-#endif
 
-  template <class... Args> void emplace(Args &&... args) {
+  template <class... Args> void emplace(Args &&...args) {
     reset();
     ::new ((void *)std::addressof(value)) T(std::forward<Args>(args)...);
     hasVal = true;
   }
 
   OptionalStorage &operator=(T const &y) {
     if (hasValue()) {
       value = y;
void reset() noexcept {
     if (hasVal) {
       value.~T();
       hasVal = false;
     }
   }
 
   constexpr bool hasValue() const noexcept { return hasVal; }
 
-  T &getValue() LLVM_LVALUE_FUNCTION noexcept {
+  T &getValue() &noexcept {
     assert(hasVal);
     return value;
   }
-  constexpr T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
+  constexpr T const &getValue() const &noexcept {
     assert(hasVal);
     return value;
   }
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
-  T &&getValue() && noexcept {
+  T &&getValue() &&noexcept {
     assert(hasVal);
     return std::move(value);
   }
-#endif
 
-  template <class... Args> void emplace(Args &&... args) {
+  template <class... Args> void emplace(Args &&...args) {
     reset();
     ::new ((void *)std::addressof(value)) T(std::forward<Args>(args)...);
     hasVal = true;
   }
 
   OptionalStorage &operator=(T const &y) {
     if (hasValue()) {
       value = y;
Optional &operator=(const T &y) {
     return *this;
   }
   Optional &operator=(const Optional &O) = default;
 
   void reset() { Storage.reset(); }
 
   constexpr const T *getPointer() const { return &Storage.getValue(); }
   T *getPointer() { return &Storage.getValue(); }
-  constexpr const T &getValue() const LLVM_LVALUE_FUNCTION {
-    return Storage.getValue();
-  }
-  T &getValue() LLVM_LVALUE_FUNCTION { return Storage.getValue(); }
+  constexpr const T &getValue() const & { return Storage.getValue(); }
+  T &getValue() & { return Storage.getValue(); }
 
   constexpr explicit operator bool() const { return hasValue(); }
   constexpr bool hasValue() const { return Storage.hasValue(); }
   constexpr const T *operator->() const { return getPointer(); }
   T *operator->() { return getPointer(); }
-  constexpr const T &operator*() const LLVM_LVALUE_FUNCTION {
-    return getValue();
-  }
-  T &operator*() LLVM_LVALUE_FUNCTION { return getValue(); }
+  constexpr const T &operator*() const & { return getValue(); }
+  T &operator*() & { return getValue(); }
 
-  template <typename U>
-  constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {
+  template <typename U> constexpr T getValueOr(U &&value) const & {
     return hasValue() ? getValue() : std::forward<U>(value);
   }
 
   /// Apply a function to the value if present; otherwise return None.
   template <class Function>
-  auto map(const Function &F) const LLVM_LVALUE_FUNCTION
-      -> Optional<decltype(F(getValue()))> {
-    if (*this) return F(getValue());
+  auto map(const Function &F) const & -> Optional<decltype(F(getValue()))> {
+    if (*this)
+      return F(getValue());
     return None;
   }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
   T &&getValue() && { return std::move(Storage.getValue()); }
   T &&operator*() && { return std::move(Storage.getValue()); }
 
-  template <typename U>
-  T getValueOr(U &&value) && {
+  template <typename U> T getValueOr(U &&value) && {
     return hasValue() ? std::move(getValue()) : std::forward<U>(value);
   }
 
   /// Apply a function to the value if present; otherwise return None.
   template <class Function>
-  auto map(const Function &F) &&
-      -> Optional<decltype(F(std::move(*this).getValue()))> {
-    if (*this) return F(std::move(*this).getValue());
+  auto map(const Function &F)
+      && -> Optional<decltype(F(std::move(*this).getValue()))> {
+    if (*this)
+      return F(std::move(*this).getValue());
     return None;
   }
-#endif
 };
 
 template <class T> llvm::hash_code hash_value(const Optional<T> &O) {
   return O ? hash_combine(true, *O) : hash_value(false);
 }
 
 template <typename T, typename U>
 constexpr bool operator==(const Optional<T> &X, const Optional<U> &Y) {

llvm/include/llvm/ADT/PointerIntPair.h

public:
   }
 
   explicit PointerIntPair(PointerTy PtrVal) { initWithPointer(PtrVal); }
 
   PointerTy getPointer() const { return Info::getPointer(Value); }
 
   IntType getInt() const { return (IntType)Info::getInt(Value); }
 
-  void setPointer(PointerTy PtrVal) LLVM_LVALUE_FUNCTION {
+  void setPointer(PointerTy PtrVal) & {
     Value = Info::updatePointer(Value, PtrVal);
   }
 
-  void setInt(IntType IntVal) LLVM_LVALUE_FUNCTION {
+  void setInt(IntType IntVal) & {
     Value = Info::updateInt(Value, static_cast<intptr_t>(IntVal));
   }
 
-  void initWithPointer(PointerTy PtrVal) LLVM_LVALUE_FUNCTION {
+  void initWithPointer(PointerTy PtrVal) & {
     Value = Info::updatePointer(0, PtrVal);
   }
 
-  void setPointerAndInt(PointerTy PtrVal, IntType IntVal) LLVM_LVALUE_FUNCTION {
+  void setPointerAndInt(PointerTy PtrVal, IntType IntVal) & {
     Value = Info::updateInt(Info::updatePointer(0, PtrVal),
                             static_cast<intptr_t>(IntVal));
   }
 
   PointerTy const *getAddrOfPointer() const {
     return const_cast<PointerIntPair *>(this)->getAddrOfPointer();
   }
 
   PointerTy *getAddrOfPointer() {
     assert(Value == reinterpret_cast<intptr_t>(getPointer()) &&
            "Can only return the address if IntBits is cleared and "
            "PtrTraits doesn't change the pointer");
     return reinterpret_cast<PointerTy *>(&Value);
   }
 
   void *getOpaqueValue() const { return reinterpret_cast<void *>(Value); }
 
-  void setFromOpaqueValue(void *Val) LLVM_LVALUE_FUNCTION {
+  void setFromOpaqueValue(void *Val) & {
     Value = reinterpret_cast<intptr_t>(Val);
   }
 
   static PointerIntPair getFromOpaqueValue(void *V) {
     PointerIntPair P;
     P.setFromOpaqueValue(V);
     return P;
   }

llvm/include/llvm/Support/Compiler.h

 #if !LLVM_MSC_PREREQ(1920)
 #error LLVM requires at least VS 2019.
 #endif
 
 #else
 #define LLVM_MSC_PREREQ(version) 0
 #endif
 
-/// Does the compiler support ref-qualifiers for *this?
-///
-/// Sadly, this is separate from just rvalue reference support because GCC
-/// and MSVC implemented this later than everything else. This appears to be
-/// corrected in MSVC 2019 but not MSVC 2017.
-/// FIXME: Remove LLVM_HAS_RVALUE_REFERENCE_THIS macro
-#define LLVM_HAS_RVALUE_REFERENCE_THIS 1
-
-/// Expands to '&' if ref-qualifiers for *this are supported.
-///
-/// This can be used to provide lvalue/rvalue overrides of member functions.
-/// The rvalue override should be guarded by LLVM_HAS_RVALUE_REFERENCE_THIS
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
-#define LLVM_LVALUE_FUNCTION &
-#else
-#define LLVM_LVALUE_FUNCTION
-#endif
-
 /// LLVM_LIBRARY_VISIBILITY - If a class marked with this attribute is linked
 /// into a shared library, then the class should be private to the library and
 /// not accessible from outside it.  Can also be used to mark variables and
 /// functions, making them private to any shared library they are linked into.
 /// On PE/COFF targets, library visibility is the default, so this isn't needed.
 ///
 /// LLVM_EXTERNAL_VISIBILITY - classes, functions, and variables marked with
 /// this attribute will be made public and visible outside of any shared library

llvm/unittests/ADT/OptionalTest.cpp

 TEST(OptionalTest, DeletedCopyStringMap) {
   // Old versions of gcc (7.3 and prior) instantiate the copy constructor when
   // std::is_trivially_copyable is instantiated.  This test will fail
   // compilation if std::is_trivially_copyable is used in the OptionalStorage
   // specialization condition by gcc <= 7.3.
   Optional<NoCopyStringMap> TestInstantiation;
 }
 
-#if LLVM_HAS_RVALUE_REFERENCE_THIS
-
 TEST(OptionalTest, MoveGetValueOr) {
   Optional<MoveOnly> A;
 
   MoveOnly::ResetCounts();
   EXPECT_EQ(42, std::move(A).getValueOr(MoveOnly(42)).val);
   EXPECT_EQ(1u, MoveOnly::MoveConstructions);
   EXPECT_EQ(0u, MoveOnly::MoveAssignments);
   EXPECT_EQ(2u, MoveOnly::Destructions);
 
   A = MoveOnly(5);
   MoveOnly::ResetCounts();
   EXPECT_EQ(5, std::move(A).getValueOr(MoveOnly(42)).val);
   EXPECT_EQ(1u, MoveOnly::MoveConstructions);
   EXPECT_EQ(0u, MoveOnly::MoveAssignments);
   EXPECT_EQ(2u, MoveOnly::Destructions);
 }
 
-#endif // LLVM_HAS_RVALUE_REFERENCE_THIS
-
 struct EqualTo {
   template <typename T, typename U> static bool apply(const T &X, const U &Y) {
     return X == Y;
   }
 };
 
 struct NotEqualTo {
   template <typename T, typename U> static bool apply(const T &X, const U &Y) {