-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[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
Conversation
@llvm/pr-subscribers-clang Author: Qizhi Hu (jcsxky) ChangesDon't skip searching in Full diff: https://github.com/llvm/llvm-project/pull/93923.diff 2 Files Affected:
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);
|
There was a problem hiding this 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()); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I have updated testcase according to your suggestion to demonstrate this solution works correctly on the testcase above. |
There was a problem hiding this 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EXPECT_FALSE(ImportedE->enumerators().empty()); |
This check is not necessary (if the other is here).
There was a problem hiding this comment.
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"))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a32f4f8
to
d8c1d25
Compare
…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).
…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).
Don't skip searching in
ToContext
during importingEnumDecl
. AndIsStructuralMatch
inStructralEquivalence
can make sure to determine whether the found result is match or not.