[ASTImporter] Remove ASTNodeImporter::IsStructuralMatch overload for EnumConstantDecl

[teemperor]

Summary

1. Moves the check to ASTStructuralEquivalence.cpp like all the other checks.
2. Adds the missing checks for identifier and init expression. Also add the respective tests for that stuff.

[teemperor]

clang/lib/AST/ASTStructuralEquivalence.cpp

1640

FWIW, there is some duplicating here that we could avoid. The question is if we should keep the diagnostics or drop them?

(This code here is also missing the InitExpr check)

[martong]

Great, thanks!

clang/lib/AST/ASTStructuralEquivalence.cpp

1640

I don't mind if we drop the warning, actually, we do emit the warnings inconsistently, so it would not matter.
(We do not emit any warning in some of the IsStructurallyEquivalent overloads at all. )

In a long term, I think, either we should emit the warnings everywhere, or we should return with the non-equivalent node pair, so the user code of StructuralEquivalenceContext could emit the warning properly and only in one place. I'd prefer the latter.

clang/lib/AST/ASTImporter.cpp

public:
 
     Expected<InheritedConstructor>
     ImportInheritedConstructor(const InheritedConstructor &From);
 
     template <typename T>
     bool hasSameVisibilityContextAndLinkage(T *Found, T *From);
 
     bool IsStructuralMatch(Decl *From, Decl *To, bool Complain = true);
-    bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl *ToEC);
     ExpectedDecl VisitDecl(Decl *D);
     ExpectedDecl VisitImportDecl(ImportDecl *D);
     ExpectedDecl VisitEmptyDecl(EmptyDecl *D);
     ExpectedDecl VisitAccessSpecDecl(AccessSpecDecl *D);
     ExpectedDecl VisitStaticAssertDecl(StaticAssertDecl *D);
     ExpectedDecl VisitTranslationUnitDecl(TranslationUnitDecl *D);
     ExpectedDecl VisitBindingDecl(BindingDecl *D);
     ExpectedDecl VisitNamespaceDecl(NamespaceDecl *D);
bool ASTNodeImporter::IsStructuralMatch(Decl *From, Decl *To, bool Complain) {
 
   StructuralEquivalenceContext Ctx(
       Importer.getFromContext(), Importer.getToContext(),
       Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
       false, Complain);
   return Ctx.IsEquivalent(From, To);
 }
 
-bool ASTNodeImporter::IsStructuralMatch(EnumConstantDecl *FromEC,
-                                        EnumConstantDecl *ToEC) {
-  const llvm::APSInt &FromVal = FromEC->getInitVal();
-  const llvm::APSInt &ToVal = ToEC->getInitVal();
-
-  return FromVal.isSigned() == ToVal.isSigned() &&
-         FromVal.getBitWidth() == ToVal.getBitWidth() &&
-         FromVal == ToVal;
-}
-
 ExpectedDecl ASTNodeImporter::VisitDecl(Decl *D) {
   Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
     << D->getDeclKindName();
   return make_error<ImportError>(ImportError::UnsupportedConstruct);
 }
 
 ExpectedDecl ASTNodeImporter::VisitImportDecl(ImportDecl *D) {
   Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)

clang/lib/AST/ASTStructuralEquivalence.cpp

if (Context.Complain) {
       Context.Diag1(D1->getLocation(), diag::note_odr_missing_field);
     }
     return false;
   }
 
   return true;
 }
 
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     EnumConstantDecl *D1,
+                                     EnumConstantDecl *D2) {
+  const llvm::APSInt &FromVal = D1->getInitVal();
+  const llvm::APSInt &ToVal = D2->getInitVal();
+  if (FromVal.isSigned() != ToVal.isSigned())
+    return false;
+  if (FromVal.getBitWidth() != ToVal.getBitWidth())
+    return false;
+  if (FromVal != ToVal)
+    return false;
+
+  if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
+    return false;
+
+  // Init expressions are the most expensive check, so do them last.
+  return IsStructurallyEquivalent(Context, D1->getInitExpr(),
+                                  D2->getInitExpr());
+}
+
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      EnumDecl *D1, EnumDecl *D2) {
 
   // Check for equivalent enum names.
   IdentifierInfo *Name1 = D1->getIdentifier();
   if (!Name1 && D1->getTypedefNameForAnonDecl())
     Name1 = D1->getTypedefNameForAnonDecl()->getIdentifier();
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   if (!D1 || !D2)
     return true;
 
   EnumDecl::enumerator_iterator EC2 = D2->enumerator_begin(),
                                 EC2End = D2->enumerator_end();
   for (EnumDecl::enumerator_iterator EC1 = D1->enumerator_begin(),
                                      EC1End = D1->enumerator_end();
        EC1 != EC1End; ++EC1, ++EC2) {
     if (EC2 == EC2End) {
       if (Context.Complain) {
         Context.Diag2(D2->getLocation(),
                       Context.getApplicableDiagnostic(
                           diag::err_odr_tag_type_inconsistent))
             << Context.ToCtx.getTypeDeclType(D2);
         Context.Diag1(EC1->getLocation(), diag::note_odr_enumerator)
             << EC1->getDeclName() << toString(EC1->getInitVal(), 10);
         Context.Diag2(D2->getLocation(), diag::note_odr_missing_enumerator);

clang/unittests/AST/StructuralEquivalenceTest.cpp

 }
 
 TEST_F(StructuralEquivalenceEnumTest, EnumsWithDifferentBody) {
   auto t = makeNamedDecls("enum class foo { B };", "enum class foo { A };",
                           Lang_CXX11);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceEnumConstantTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceEnumConstantTest, EnumConstantsWithSameValues) {
+  auto t = makeNamedDecls("enum foo { foo = 1 };", "enum foo { foo = 1 };",
+                          Lang_C89);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest,
+       EnumConstantsWithDifferentValues) {
+  auto t =
+      makeNamedDecls("enum e { foo = 1 };", "enum e { foo = 2 };", Lang_C89);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest,
+       EnumConstantsWithDifferentExprsButSameValues) {
+  auto t = makeNamedDecls("enum e { foo = 1 + 1 };", "enum e { foo = 2 };",
+                          Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest,
+       EnumConstantsWithDifferentSignedness) {
+  auto t = makeNamedDecls("enum e : unsigned { foo = 1 };",
+                          "enum e : int { foo = 1 };", Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest, EnumConstantsWithDifferentWidth) {
+  auto t = makeNamedDecls("enum e : short { foo = 1 };",
+                          "enum e : int { foo = 1 };", Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest, EnumConstantsWithDifferentName) {
+  auto t =
+      makeDecls<EnumConstantDecl>("enum e { foo = 1 };", "enum e { bar = 1 };",
+                                  Lang_CXX11, enumConstantDecl());
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 struct StructuralEquivalenceTemplateTest : StructuralEquivalenceTest {};
 
 TEST_F(StructuralEquivalenceTemplateTest, ExactlySameTemplates) {
   auto t = makeNamedDecls("template <class T> struct foo;",
                           "template <class T> struct foo;", Lang_CXX03);
   EXPECT_TRUE(testStructuralMatch(t));
 }