Skip to content

[clang][ASTImport] fix issue on anonymous enum import #93923

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

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

jcsxky
Copy link
Contributor

@jcsxky jcsxky commented May 31, 2024

Don't skip searching in ToContext during importing EnumDecl. And IsStructuralMatch in StructralEquivalence can make sure to determine whether the found result is match or not.

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

llvmbot commented May 31, 2024

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Don't skip searching in ToContext during importing EnumDecl. And IsStructuralMatch in StructralEquivalence can make sure to determine whether the found result is match or not.


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

2 Files Affected:

  • (modified) clang/lib/AST/ASTImporter.cpp (+1-1)
  • (modified) clang/unittests/AST/ASTImporterTest.cpp (+21)
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index cab5ee6047956..3b9080e09b331 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -2929,7 +2929,7 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
 
   // We may already have an enum of the same name; try to find and match it.
   EnumDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod() && SearchName) {
+  if (!DC->isFunctionOrMethod()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     auto FoundDecls =
         Importer.findDeclsInToCtx(DC, SearchName);
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 3dc1c336365d1..4cee0b75653a5 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -9674,6 +9674,27 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportInstantiatedFromMember) {
   EXPECT_TRUE(ImportedPartialSpecialization->getInstantiatedFromMember());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousEnum) {
+  const char *ToCode =
+      R"(
+      struct A {
+        enum { E1,E2} x;
+      };
+      )";
+  (void)getToTuDecl(ToCode, Lang_CXX11);
+  const char *Code =
+      R"(
+      struct A {
+        enum { E1,E2} x;
+      };
+      )";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
+  auto *FromE = FirstDeclMatcher<EnumDecl>().match(FromTU, enumDecl());
+  auto *ToE = Import(FromE, Lang_CXX11);
+  ASSERT_TRUE(ToE);
+  EXPECT_FALSE(ToE->enumerators().empty());
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 

@jcsxky jcsxky requested a review from balazske May 31, 2024 06:10
Copy link
Collaborator

@balazske balazske left a comment

Choose a reason for hiding this comment

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

Does this work on the following code?

     struct A {
        enum { E1,E2 } x;
        enum { E3,E4 } y;
      };

auto *FromE = FirstDeclMatcher<EnumDecl>().match(FromTU, enumDecl());
auto *ToE = Import(FromE, Lang_CXX11);
ASSERT_TRUE(ToE);
EXPECT_FALSE(ToE->enumerators().empty());
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is more safe to get the EnumDecl in the ToTU before the import (ToExistingE) and after import check if it is equal to ToE.

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.

@jcsxky
Copy link
Contributor Author

jcsxky commented May 31, 2024

Does this work on the following code?

     struct A {
        enum { E1,E2 } x;
        enum { E3,E4 } y;
      };

I have updated testcase according to your suggestion to demonstrate this solution works correctly on the testcase above.

Copy link
Collaborator

@balazske balazske left a comment

Choose a reason for hiding this comment

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

The change looks good, only the test can be made more exact.

auto *ImportedE = Import(FromE, Lang_CXX11);
ASSERT_TRUE(ImportedE);
EXPECT_EQ(ImportedE, ToE);
EXPECT_FALSE(ImportedE->enumerators().empty());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
EXPECT_FALSE(ImportedE->enumerators().empty());

This check is not necessary (if the other is here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

)";
Decl *ToTu = getToTuDecl(ToCode, Lang_CXX11);
auto *ToE = FirstDeclMatcher<EnumDecl>().match(
ToTu, enumDecl(hasEnumConstName("E1")));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to have a similar check (and a new import and check) with the other enum. This ensures that the enums are not mixed somehow by the import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Import test on the other enum has been added to make this test completely.

@jcsxky jcsxky force-pushed the fix-anonymous-enum-import branch from a32f4f8 to d8c1d25 Compare June 3, 2024 13:57
@jcsxky jcsxky merged commit 3b020d5 into llvm:main Jun 4, 2024
7 checks passed
balazske added a commit that referenced this pull request Jul 23, 2024
…esent (#99281)

After changes in PR #87144 and #93923 regressions appeared in some
cases. The problem was that if multiple anonymous enums are present in a
class and are imported as new the import of the second enum can fail
because it is detected as different from the first and causes ODR error.

Now in case of enums without name an existing similar enum is searched,
if not found the enum is imported. ODR error is not detected. This may
be incorrect if non-matching structures are imported, but this is the
less important case (import of matching classes is more important to
work).
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…esent (#99281)

After changes in PR #87144 and #93923 regressions appeared in some
cases. The problem was that if multiple anonymous enums are present in a
class and are imported as new the import of the second enum can fail
because it is detected as different from the first and causes ODR error.

Now in case of enums without name an existing similar enum is searched,
if not found the enum is imported. ODR error is not detected. This may
be incorrect if non-matching structures are imported, but this is the
less important case (import of matching classes is more important to
work).
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
Development

Successfully merging this pull request may close these issues.

3 participants