Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][ASTImporter][StructuralEquivalence] improve StructuralEquivalence on recordType #76226

Merged

Conversation

jcsxky
Copy link
Contributor

@jcsxky jcsxky commented Dec 22, 2023

Types comparison in StructuralEquivalence ignores its DeclContext when they are generated by template specialization implicitly and this will produce incorrect result. Add comparison of DeclContext of ClassTemplateSpecializationDecl to improve result.
fix issue

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 22, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 22, 2023

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Types comparison in StructuralEquivalence ignores its DeclContext when they are generated by template specialization implicitly and this will produce incorrect result. Add comparison of DeclContext of ClassTemplateSpecializationDecl to improve result.


Full diff: https://github.com/llvm/llvm-project/pull/76226.diff

2 Files Affected:

  • (modified) clang/lib/AST/ASTStructuralEquivalence.cpp (+12-3)
  • (modified) clang/unittests/AST/StructuralEquivalenceTest.cpp (+22)
diff --git a/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp
index 6bb4bf14b873d7..7c04c09bb80874 100644
--- a/clang/lib/AST/ASTStructuralEquivalence.cpp
+++ b/clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1107,11 +1107,20 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   }
 
   case Type::Record:
-  case Type::Enum:
-    if (!IsStructurallyEquivalent(Context, cast<TagType>(T1)->getDecl(),
-                                  cast<TagType>(T2)->getDecl()))
+  case Type::Enum: {
+    auto *D1 = cast<TagType>(T1)->getDecl();
+    auto *D2 = cast<TagType>(T2)->getDecl();
+    if (!IsStructurallyEquivalent(Context, D1, D2))
+      return false;
+    auto *D1Spec =
+        dyn_cast_or_null<ClassTemplateSpecializationDecl>(D1->getDeclContext());
+    auto *D2Spec =
+        dyn_cast_or_null<ClassTemplateSpecializationDecl>(D2->getDeclContext());
+    if (nullptr != D1Spec && nullptr != D2Spec &&
+        !IsStructurallyEquivalent(Context, D1Spec, D2Spec))
       return false;
     break;
+  }
 
   case Type::TemplateTypeParm: {
     const auto *Parm1 = cast<TemplateTypeParmType>(T1);
diff --git a/clang/unittests/AST/StructuralEquivalenceTest.cpp b/clang/unittests/AST/StructuralEquivalenceTest.cpp
index 44d950cfe758f1..b54d149152e105 100644
--- a/clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ b/clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -1024,6 +1024,28 @@ TEST_F(StructuralEquivalenceRecordContextTest, TransparentContextInNamespace) {
   EXPECT_TRUE(testStructuralMatch(Decls));
 }
 
+TEST_F(StructuralEquivalenceRecordContextTest, RecordWithinTemplateClass) {
+  std::string Code =
+      R"(
+      template <typename T> struct O {
+        struct M {};
+      };
+      )";
+  auto t = makeDecls<VarDecl>(Code + R"(
+      typedef O<int>::M MT1;
+      MT1 A;
+      )",
+                              Code + R"(
+      namespace {
+        struct I {};
+      } // namespace
+      typedef O<I>::M MT2;
+      MT2 A;
+      )",
+                              Lang_CXX11, varDecl(hasName("A")));
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTest, NamespaceOfRecordMember) {
   auto Decls = makeNamedDecls(
       R"(

@jcsxky jcsxky force-pushed the improve_StructuralEquivalence_on_record_type branch 4 times, most recently from be62fb6 to 574fa37 Compare December 23, 2023 02:53
@jcsxky jcsxky force-pushed the improve_StructuralEquivalence_on_record_type branch from 574fa37 to 27005db Compare December 23, 2023 07:56
@jcsxky jcsxky force-pushed the improve_StructuralEquivalence_on_record_type branch from 27005db to 7797602 Compare December 23, 2023 08:08
@@ -1491,6 +1492,12 @@ static bool IsRecordContextStructurallyEquivalent(RecordDecl *D1,
return false;
}

if (auto *D1Spec = dyn_cast<ClassTemplateSpecializationDecl>(DC1)) {
auto *D2Spec = dyn_cast<ClassTemplateSpecializationDecl>(DC2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to check if D2Spec is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that the old code was better, there is a previous check DC1->getDeclKind() != DC2->getDeclKind(), this should ensure that the type is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

@jcsxky jcsxky force-pushed the improve_StructuralEquivalence_on_record_type branch 2 times, most recently from ab1ace9 to 7797602 Compare January 3, 2024 09:45
@jcsxky jcsxky merged commit 8612730 into llvm:main Jan 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
4 participants