martong created this revision.
martong added reviewers: a_sidorin, shafik, teemperor.
Herald added subscribers: cfe-commits, jdoerfert, gamesh411, Szelethus, dkrupp,
rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.
martong added a parent revision: D55049: Changed every use of
ASTImporter::Import to Import_New.
There are numorous flaws about the name conflict handling, this patch
attempts fixes them. Changes in details:
- HandleNameConflict return with a false DeclarationName
Hitherto we effectively never returned with a NameConflict error, even
if the preceding StructuralMatch indicated a conflict.
Because we just simply returned with the parameter `Name` in
HandleNameConflict and that name is almost always `true` when converted to
`bool`.
- Add tests which indicate wrong NameConflict handling
- Add to ConflictingDecls only if decl kind is different
Note, we might not indicate an ODR error when there is an existing record decl
and a enum is imported with same name. But there are other cases. E.g. think
about the case when we import a FunctionTemplateDecl with name f and we found a
simple FunctionDecl with name f. They overload. Or in case of a
ClassTemplateDecl and CXXRecordDecl, the CXXRecordDecl could be the 'templated'
class, so it would be false to report error. So I think we should report a
name conflict error only when we are 100% sure of that. That is why I think it
should be a general pattern to report the error only if the kind is the same.
- Fix failing ctu test with EnumConstandDecl
In ctu-main.c we have the enum class 'A' which brings in the enum
constant 'x' with value 0 into the global namespace.
In ctu-other.c we had the enum class 'B' which brought in the same name
('x') as an enum constant but with a different enum value (42). This is clearly
an ODR violation in the global namespace. The solution was to rename the
second enum constant.
- Fix lldb test failures
Remove the call of the unused and vexing GetOriginalDecl(). This information is
already maintained in the ImportedDecls member.
Repository:
rC Clang
https://reviews.llvm.org/D59692
Files:
include/clang/AST/ASTImporter.h
lib/AST/ASTImporter.cpp
test/Analysis/Inputs/ctu-other.c
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===================================================================
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1964,7 +1964,7 @@
auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
// We expect one (ODR) warning during the import.
EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
- EXPECT_EQ(2u,
+ EXPECT_EQ(1u,
DeclCounter<RecordDecl>().match(ToTU, recordDecl(hasName("X"))));
}
@@ -2674,6 +2674,64 @@
functionDecl(hasName("f"), hasDescendant(declRefExpr()))))));
}
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportFunctionTemplates,
+ ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+ getToTuDecl(
+ R"(
+ template <typename T>
+ void foo(T) {}
+ void foo();
+ )",
+ Lang_CXX);
+ Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+ auto *FromD = FirstDeclMatcher<FunctionDecl>().match(
+ FromTU, functionDecl(hasName("foo")));
+ auto *ImportedD = Import(FromD, Lang_CXX);
+ EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+ ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+ auto Code =
+ R"(
+ struct Foo {
+ template <typename T>
+ Foo(T) {}
+ Foo();
+ };
+ )";
+ getToTuDecl(Code, Lang_CXX);
+ Decl *FromTU = getTuDecl(Code, Lang_CXX);
+ auto *FromD =
+ LastDeclMatcher<CXXConstructorDecl>().match(FromTU, cxxConstructorDecl());
+ auto *ImportedD = Import(FromD, Lang_CXX);
+ EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+ ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+ getToTuDecl(
+ R"(
+ template <typename T>
+ void operator<(T,T) {}
+ struct X{};
+ void operator<(X, X);
+ )",
+ Lang_CXX);
+ Decl *FromTU = getTuDecl(
+ R"(
+ struct X{};
+ void operator<(X, X);
+ )",
+ Lang_CXX);
+ auto *FromD = LastDeclMatcher<FunctionDecl>().match(
+ FromTU, functionDecl(hasOverloadedOperatorName("<")));
+ auto *ImportedD = Import(FromD, Lang_CXX);
+ EXPECT_TRUE(ImportedD);
+}
+
struct ImportFriendFunctions : ImportFunctions {};
TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5534,6 +5592,9 @@
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
DefaultTestValuesForRunOptions, );
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+ DefaultTestValuesForRunOptions, );
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
DefaultTestValuesForRunOptions, );
Index: test/Analysis/Inputs/ctu-other.c
===================================================================
--- test/Analysis/Inputs/ctu-other.c
+++ test/Analysis/Inputs/ctu-other.c
@@ -12,11 +12,11 @@
}
// Test enums.
-enum B { x = 42,
- l,
- s };
+enum B { x2 = 42,
+ y2,
+ z2 };
int enumCheck(void) {
- return x;
+ return x2;
}
// Test reporting an error in macro definition
Index: lib/AST/ASTImporter.cpp
===================================================================
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1944,15 +1944,6 @@
bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
RecordDecl *ToRecord, bool Complain) {
- // Eliminate a potential failure point where we attempt to re-import
- // something we're trying to import while completing ToRecord.
- Decl *ToOrigin = Importer.GetOriginalDecl(ToRecord);
- if (ToOrigin) {
- auto *ToOriginRecord = dyn_cast<RecordDecl>(ToOrigin);
- if (ToOriginRecord)
- ToRecord = ToOriginRecord;
- }
-
StructuralEquivalenceContext Ctx(Importer.getFromContext(),
ToRecord->getASTContext(),
Importer.getNonEquivalentDecls(),
@@ -2262,9 +2253,8 @@
// already have a complete underlying type then return with that.
if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
return Importer.MapImported(D, FoundTypedef);
- }
- // FIXME Handle redecl chain.
- break;
+ } else
+ ConflictingDecls.push_back(FoundDecl);
}
ConflictingDecls.push_back(FoundDecl);
@@ -2454,9 +2444,8 @@
if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
if (IsStructuralMatch(D, FoundEnum))
return Importer.MapImported(D, FoundEnum);
+ ConflictingDecls.push_back(FoundDecl);
}
-
- ConflictingDecls.push_back(FoundDecl);
}
if (!ConflictingDecls.empty()) {
@@ -2586,9 +2575,8 @@
PrevDecl = FoundRecord->getMostRecentDecl();
break;
}
- }
-
- ConflictingDecls.push_back(FoundDecl);
+ ConflictingDecls.push_back(FoundDecl);
+ } // kind is RecordDecl
} // for
if (!ConflictingDecls.empty() && SearchName) {
@@ -2755,9 +2743,8 @@
if (auto *FoundEnumConstant = dyn_cast<EnumConstantDecl>(FoundDecl)) {
if (IsStructuralMatch(D, FoundEnumConstant))
return Importer.MapImported(D, FoundEnumConstant);
+ ConflictingDecls.push_back(FoundDecl);
}
-
- ConflictingDecls.push_back(FoundDecl);
}
if (!ConflictingDecls.empty()) {
@@ -2984,9 +2971,8 @@
<< Name << D->getType() << FoundFunction->getType();
Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
<< FoundFunction->getType();
+ ConflictingDecls.push_back(FoundDecl);
}
-
- ConflictingDecls.push_back(FoundDecl);
}
if (!ConflictingDecls.empty()) {
@@ -3606,9 +3592,8 @@
<< Name << D->getType() << FoundVar->getType();
Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
<< FoundVar->getType();
+ ConflictingDecls.push_back(FoundDecl);
}
-
- ConflictingDecls.push_back(FoundDecl);
}
if (!ConflictingDecls.empty()) {
@@ -4942,19 +4927,17 @@
FoundByLookup = FoundTemplate;
break;
}
+ ConflictingDecls.push_back(FoundDecl);
}
-
- ConflictingDecls.push_back(FoundDecl);
}
if (!ConflictingDecls.empty()) {
Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
ConflictingDecls.data(),
ConflictingDecls.size());
+ if (!Name)
+ return make_error<ImportError>(ImportError::NameConflict);
}
-
- if (!Name)
- return make_error<ImportError>(ImportError::NameConflict);
}
CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
@@ -5221,22 +5204,18 @@
FoundTemplate->getTemplatedDecl());
return Importer.MapImported(D, FoundTemplate);
}
+ ConflictingDecls.push_back(FoundDecl);
}
-
- ConflictingDecls.push_back(FoundDecl);
}
if (!ConflictingDecls.empty()) {
Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
ConflictingDecls.data(),
ConflictingDecls.size());
+ if (!Name)
+ return make_error<ImportError>(ImportError::NameConflict);
}
- if (!Name)
- // FIXME: Is it possible to get other error than name conflict?
- // (Put this `if` into the previous `if`?)
- return make_error<ImportError>(ImportError::NameConflict);
-
VarDecl *DTemplated = D->getTemplatedDecl();
// Import the type.
@@ -8550,7 +8529,7 @@
unsigned IDNS,
NamedDecl **Decls,
unsigned NumDecls) {
- return Name;
+ return DeclarationName();
}
Selector ASTImporter::Import(Selector From) {
llvm::Expected<Selector> To = Import_New(From);
Index: include/clang/AST/ASTImporter.h
===================================================================
--- include/clang/AST/ASTImporter.h
+++ include/clang/AST/ASTImporter.h
@@ -430,6 +430,7 @@
/// Store and assign the imported declaration to its counterpart.
Decl *MapImported(Decl *From, Decl *To);
+ /// Deprecated. FIXME use [[deprecated]] once Clang enables C++14.
/// Called by StructuralEquivalenceContext. If a RecordDecl is
/// being compared to another RecordDecl as part of import, completing the
/// other RecordDecl may trigger importation of the first RecordDecl. This
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
