[clang] Fix warnings when `-Wdeprecated-enum-enum-conversion` is enabled

[antoniofrighetto]

Summary

clang may throw the following warning: include/clang/AST/DeclarationName.h:210:52: error: arithmetic between different enumeration types ('clang::DeclarationName::StoredNameKind' and 'clang::detail::DeclarationNameExtra::ExtraKind') is deprecated when flags -Werror,-Wdeprecated-enum-enum-conversion are on.

[aaron.ballman]

The changes here look correct to me, but they complicate the code a reasonable amount. I almost wonder whether we want to add a helper function (perhaps even to STLExtras.h?) along the lines of a cleaned up version of:

template <typename EnumTy1, typename EnumTy2>
auto addEnumValues(EnumTy1 LHS, EnumTy2 RHS) {
  return static_cast<std::underlying_type_t<EnumTy1>>(LHS) +
    static_cast<std::underlying_type_t<EnumTy2>>(RHS);
}

(We'd probably want some enable_if magic to protect the interface a bit more as well). WDYT of something like that? (That change would require some unit testing coverage as well, but this strikes me as something we're likely to want to reuse given that the functionality is deprecated.)

[antoniofrighetto]

Looks definitely better! How about this slightly changed version protecting the interface?

/// Helper which adds two underlying types of enumeration type.
template <typename EnumTy1,
          typename EnumTy2,
          typename UT1 = std::enable_if_t<std::is_enum_v<EnumTy1>, std::underlying_type_t<EnumTy1>>,
          typename UT2 = std::enable_if_t<std::is_enum_v<EnumTy2>, std::underlying_type_t<EnumTy2>>>
constexpr auto addEnumValues(EnumTy1 LHS, EnumTy2 RHS) {
  return static_cast<UT1>(LHS) + static_cast<UT2>(RHS);
}

I feel like this is something we may wish to readopt in the future for other similar cases as well (e.g., -Wdeprecated-anon-enum-enum-conversion in Comment.h).

[aaron.ballman]

In D122808#3425703, @antoniofrighetto wrote:

Looks definitely better! How about this slightly changed version protecting the interface?

/// Helper which adds two underlying types of enumeration type.
template <typename EnumTy1,
          typename EnumTy2,
          typename UT1 = std::enable_if_t<std::is_enum_v<EnumTy1>, std::underlying_type_t<EnumTy1>>,
          typename UT2 = std::enable_if_t<std::is_enum_v<EnumTy2>, std::underlying_type_t<EnumTy2>>>
constexpr auto addEnumValues(EnumTy1 LHS, EnumTy2 RHS) {
  return static_cast<UT1>(LHS) + static_cast<UT2>(RHS);
}

I feel like this is something we may wish to readopt in the future for other similar cases as well (e.g., -Wdeprecated-anon-enum-enum-conversion in Comment.h).

I think that's a great interface for it!

[aaron.ballman]

Thanks! This is heading in the right direction. You should also add some test coverage to llvm/unittests/ADT/STLExtrasTests.cpp for the new interface.

llvm/include/llvm/ADT/STLExtras.h

207–211 ↗

(On Diff #420458)

Oops, but is_enum_v<> is C++17 and up, and we currently still require only C++14, so this will have to use the long-hand form (precommit CI was failing because of this).

[antoniofrighetto]

Fixed the change, and added a new test. Hope the coverage is at a fairly good level now!

[aaron.ballman]

llvm/unittests/ADT/STLExtrasTest.cpp

994 ↗

(On Diff #420957)

It looks like you need to include <climits> for the macros.

[antoniofrighetto]

llvm/unittests/ADT/STLExtrasTest.cpp

994 ↗

(On Diff #420957)

Right, saw the CI, fixed, thanks! :)

[aaron.ballman]

LGTM, thank you for the new interface and fixes! (Current Precommit CI failures look unrelated to me.)

[antoniofrighetto]

Awesome, thank you for reviewing this! PS: I do not have commit access, feel free to close it, if you say.

[aaron.ballman]

In D122808#3435821, @antoniofrighetto wrote:

Awesome, thank you for reviewing this! PS: I do not have commit access, feel free to close it, if you say.

Ah, thank you for letting me know you don't have commit access. I'm happy to land this on your behalf. What name and email address would you like me to use for patch attribution?

[aaron.ballman]

Thank you for the fix! I've landed on your behalf in 7c3d8c8977cfc013254783b85b9fc6026566b35f.

clang/include/clang/AST/DeclarationName.h

 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/type_traits.h"
 #include <cassert>
 #include <cstdint>
 #include <cstring>
 #include <string>
+#include <type_traits>
 
 namespace clang {
 
 class ASTContext;
 template <typename> class CanQual;
 class DeclarationName;
 class DeclarationNameTable;
 struct PrintingPolicy;
static_assert(alignof(IdentifierInfo) >= 8 &&
                     alignof(detail::DeclarationNameExtra) >= 8 &&
                     alignof(detail::CXXSpecialNameExtra) >= 8 &&
                     alignof(detail::CXXOperatorIdName) >= 8 &&
                     alignof(detail::CXXDeductionGuideNameExtra) >= 8 &&
                     alignof(detail::CXXLiteralOperatorIdName) >= 8,
                 "The various classes that DeclarationName::Ptr can point to"
                 " must be at least aligned to 8 bytes!");
 
+  static_assert(
+      std::is_same<std::underlying_type_t<StoredNameKind>,
+                   std::underlying_type_t<
+                       detail::DeclarationNameExtra::ExtraKind>>::value,
+      "The various enums used to compute values for NameKind should all have "
+      "the same underlying type");
+
 public:
   /// The kind of the name stored in this DeclarationName.
   /// The first 7 enumeration values are stored inline and correspond
   /// to frequently used kinds. The rest is stored in DeclarationNameExtra
   /// and correspond to infrequently used kinds.
   enum NameKind {
     Identifier = StoredIdentifier,
     ObjCZeroArgSelector = StoredObjCZeroArgSelector,
     ObjCOneArgSelector = StoredObjCOneArgSelector,
     CXXConstructorName = StoredCXXConstructorName,
     CXXDestructorName = StoredCXXDestructorName,
     CXXConversionFunctionName = StoredCXXConversionFunctionName,
     CXXOperatorName = StoredCXXOperatorName,
-    CXXDeductionGuideName = UncommonNameKindOffset +
-                            detail::DeclarationNameExtra::CXXDeductionGuideName,
+    CXXDeductionGuideName =
+        static_cast<std::underlying_type_t<StoredNameKind>>(
+            UncommonNameKindOffset) +
+        static_cast<
+            std::underlying_type_t<detail::DeclarationNameExtra::ExtraKind>>(
+            detail::DeclarationNameExtra::CXXDeductionGuideName),
     CXXLiteralOperatorName =
-        UncommonNameKindOffset +
-        detail::DeclarationNameExtra::CXXLiteralOperatorName,
-    CXXUsingDirective = UncommonNameKindOffset +
-                        detail::DeclarationNameExtra::CXXUsingDirective,
-    ObjCMultiArgSelector = UncommonNameKindOffset +
-                           detail::DeclarationNameExtra::ObjCMultiArgSelector
+        static_cast<std::underlying_type_t<StoredNameKind>>(
+            UncommonNameKindOffset) +
+        static_cast<
+            std::underlying_type_t<detail::DeclarationNameExtra::ExtraKind>>(
+            detail::DeclarationNameExtra::CXXLiteralOperatorName),
+    CXXUsingDirective =
+        static_cast<std::underlying_type_t<StoredNameKind>>(
+            UncommonNameKindOffset) +
+        static_cast<
+            std::underlying_type_t<detail::DeclarationNameExtra::ExtraKind>>(
+            detail::DeclarationNameExtra::CXXUsingDirective),
+    ObjCMultiArgSelector =
+        static_cast<std::underlying_type_t<StoredNameKind>>(
+            UncommonNameKindOffset) +
+        static_cast<
+            std::underlying_type_t<detail::DeclarationNameExtra::ExtraKind>>(
+            detail::DeclarationNameExtra::ObjCMultiArgSelector),
   };
 
 private:
   /// The lowest three bits of Ptr are used to express what kind of name
   /// we're actually storing, using the values of StoredNameKind. Depending
   /// on the kind of name this is, the upper bits of Ptr may have one
   /// of several different meanings:
   ///