balazske updated this revision to Diff 422139.
balazske added a comment.
Created a common place for the `DeclContext` child error handling code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122525/new/
https://reviews.llvm.org/D122525
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5821,6 +5821,53 @@
EXPECT_FALSE(ImportedF);
}
+TEST_P(ErrorHandlingTest, DoNotInheritErrorFromNonDependentChild) {
+ // Declarations should not inherit an import error from a child object
+ // if the declaration has no direct dependence to such a child.
+ // For example a namespace should not get import error if one of the
+ // declarations inside it fails to import.
+ // There was a special case in error handling (when "import path circles" are
+ // encountered) when this property was not held. This case is provoked by the
+ // following code.
+ constexpr auto ToTUCode = R"(
+ namespace ns {
+ struct Err {
+ char A;
+ };
+ }
+ )";
+ constexpr auto FromTUCode = R"(
+ namespace ns {
+ struct A {
+ using U = struct Err;
+ };
+ }
+ namespace ns {
+ struct Err {}; // ODR violation
+ void f(A) {}
+ }
+ )";
+
+ Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+ static_cast<void>(ToTU);
+ Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+ auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("A"), hasDefinition()));
+ ASSERT_TRUE(FromA);
+ auto *ImportedA = Import(FromA, Lang_CXX11);
+ // 'A' can not be imported: ODR error at 'Err'
+ EXPECT_FALSE(ImportedA);
+ // When import of 'A' failed there was a "saved import path circle" that
+ // contained namespace 'ns' (A - U - Err - ns - f - A). This should not mean
+ // that every object in this path fails to import.
+
+ Decl *FromNS = FirstDeclMatcher<NamespaceDecl>().match(
+ FromTU, namespaceDecl(hasName("ns")));
+ EXPECT_TRUE(FromNS);
+ auto *ImportedNS = Import(FromNS, Lang_CXX11);
+ EXPECT_TRUE(ImportedNS);
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
Decl *FromTU = getTuDecl(
R"(
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -138,6 +138,46 @@
To->setIsUsed();
}
+ /// How to handle import errors that occur when import of a child declaration
+ /// of a DeclContext fails.
+ class ChildErrorHandlingStrategy {
+ /// This context is imported (in the 'from' domain).
+ /// It is nullptr if a non-DeclContext is imported.
+ const DeclContext *const FromDC;
+ /// Ignore import errors of the children.
+ /// If true, the context can be imported successfully if a child
+ /// of it failed to import. Otherwise the import errors of the child nodes
+ /// are accumulated (joined) into the import error object of the parent.
+ /// (Import of a parent can fail in other ways.)
+ bool const IgnoreChildErrors;
+
+ public:
+ ChildErrorHandlingStrategy(const DeclContext *FromDC)
+ : FromDC(FromDC), IgnoreChildErrors(!isa<TagDecl>(FromDC)) {}
+ ChildErrorHandlingStrategy(const Decl *FromD)
+ : FromDC(dyn_cast<DeclContext>(FromD)),
+ IgnoreChildErrors(!isa<TagDecl>(FromD)) {}
+
+ /// Process the import result of a child (of the current declaration).
+ /// \param ResultErr The import error that can be used as result of
+ /// importing the parent. This may be changed by the function.
+ /// \param ChildErr Result of importing a child. Can be success or error.
+ void handleChildImportResult(Error &ResultErr, Error &&ChildErr) {
+ if (ChildErr && !IgnoreChildErrors)
+ ResultErr = joinErrors(std::move(ResultErr), std::move(ChildErr));
+ else
+ consumeError(std::move(ChildErr));
+ }
+
+ /// Determine if import failure of a child does not cause import failure of
+ /// its parent.
+ bool ignoreChildErrorOnParent(Decl *FromChildD) const {
+ if (!IgnoreChildErrors || !FromDC)
+ return false;
+ return FromDC->containsDecl(FromChildD);
+ }
+ };
+
class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, ExpectedType>,
public DeclVisitor<ASTNodeImporter, ExpectedDecl>,
public StmtVisitor<ASTNodeImporter, ExpectedStmt> {
@@ -1809,7 +1849,7 @@
// because there is an ODR error with two typedefs. As another example,
// the client may allow EnumConstantDecls with same names but with
// different values in two distinct translation units.
- bool AccumulateChildErrors = isa<TagDecl>(FromDC);
+ ChildErrorHandlingStrategy HandleChildErrors(FromDC);
Error ChildErrors = Error::success();
for (auto *From : FromDC->decls()) {
@@ -1849,20 +1889,14 @@
if (FromRecordDecl->isCompleteDefinition() &&
!ToRecordDecl->isCompleteDefinition()) {
Error Err = ImportDefinition(FromRecordDecl, ToRecordDecl);
-
- if (Err && AccumulateChildErrors)
- ChildErrors = joinErrors(std::move(ChildErrors), std::move(Err));
- else
- consumeError(std::move(Err));
+ HandleChildErrors.handleChildImportResult(ChildErrors,
+ std::move(Err));
}
}
}
} else {
- if (AccumulateChildErrors)
- ChildErrors =
- joinErrors(std::move(ChildErrors), ImportedOrErr.takeError());
- else
- consumeError(ImportedOrErr.takeError());
+ HandleChildErrors.handleChildImportResult(ChildErrors,
+ ImportedOrErr.takeError());
}
}
@@ -8796,8 +8830,20 @@
// Set the error for all nodes which have been created before we
// recognized the error.
- for (const auto &Path : SavedImportPaths[FromD])
+ for (const auto &Path : SavedImportPaths[FromD]) {
+ // The import path contains import-dependency nodes first.
+ // Save the node that was imported as dependency of the current node.
+ Decl *PrevFromDi = FromD;
for (Decl *FromDi : Path) {
+ // Begin and end of the path equals 'FromD', skip it.
+ if (FromDi == FromD)
+ continue;
+ // We should not set import error on a node and all following nodes in
+ // the path if child import errors are ignored.
+ if (ChildErrorHandlingStrategy(FromDi).ignoreChildErrorOnParent(
+ PrevFromDi))
+ break;
+ PrevFromDi = FromDi;
setImportDeclError(FromDi, ErrOut);
//FIXME Should we remove these Decls from ImportedDecls?
// Set the error for the mapped to Decl, which is in the "to" context.
@@ -8807,6 +8853,7 @@
// FIXME Should we remove these Decls from the LookupTable,
// and from ImportedFromDecls?
}
+ }
SavedImportPaths.erase(FromD);
// Do not return ToDOrErr, error was taken out of it.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
