[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
szepet closed this revision. szepet added a comment. This patch was commited in r302975. (https://reviews.llvm.org/rL302975) https://reviews.llvm.org/D32981 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
xazax.hun added a comment. Is this blocked on something? https://reviews.llvm.org/D32981 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
spyffe marked 2 inline comments as done. spyffe added inline comments. Comment at: tools/clang-import-test/clang-import-test.cpp:263 +AddExternalSource(*CI, Imports); + } bruno wrote: > No need for the curly braces here Okay. https://reviews.llvm.org/D32981 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
bruno accepted this revision. bruno added a comment. Hi Sean, LGTM! One comment below. Comment at: tools/clang-import-test/clang-import-test.cpp:263 +AddExternalSource(*CI, Imports); + } No need for the curly braces here https://reviews.llvm.org/D32981 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
spyffe marked an inline comment as done.
spyffe added inline comments.
Comment at: lib/AST/ASTImporter.cpp:1757
D2->setAnonymousStructOrUnion(true);
+if (PrevDecl) {
+ D2->setPreviousDecl(PrevDecl);
a.sidorin wrote:
> By the way, should we do the same for some other kinds of Decls (like
> FunctionDecl)? If so, could you write a NOTE comment?
I added this as a `FIXME`, in the style of the other notes about incomplete
implementation in this file.
https://reviews.llvm.org/D32981
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
spyffe updated this revision to Diff 98497.
spyffe added a comment.
• Added a FIXME per Aleksei Sidorin's suggestion.
https://reviews.llvm.org/D32981
Files:
include/clang/AST/ExternalASTMerger.h
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
test/Import/conflicting-struct/Inputs/S1.cpp
test/Import/conflicting-struct/Inputs/S2.cpp
test/Import/conflicting-struct/test.cpp
tools/clang-import-test/clang-import-test.cpp
Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -42,6 +42,10 @@
Imports("import", llvm::cl::ZeroOrMore,
llvm::cl::desc("Path to a file containing declarations to import"));
+static llvm::cl::opt
+Direct("direct", llvm::cl::Optional,
+ llvm::cl::desc("Use the parsed declarations without indirection"));
+
static llvm::cl::list
ClangArgs("Xcc", llvm::cl::ZeroOrMore,
llvm::cl::desc("Argument to pass to the CompilerInvocation"),
@@ -172,6 +176,14 @@
return Ins;
}
+std::unique_ptr
+BuildCompilerInstance(ArrayRef ClangArgs) {
+ std::vector ClangArgv(ClangArgs.size());
+ std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); });
+ return init_convenience::BuildCompilerInstance(ClangArgv);
+}
+
std::unique_ptr
BuildASTContext(CompilerInstance , SelectorTable , Builtin::Context ) {
auto AST = llvm::make_unique(
@@ -205,6 +217,21 @@
CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
}
+std::unique_ptr BuildIndirect(std::unique_ptr ) {
+ std::vector ClangArgv(ClangArgs.size());
+ std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); });
+ std::unique_ptr IndirectCI =
+ init_convenience::BuildCompilerInstance(ClangArgv);
+ auto ST = llvm::make_unique();
+ auto BC = llvm::make_unique();
+ std::unique_ptr AST =
+ init_convenience::BuildASTContext(*IndirectCI, *ST, *BC);
+ IndirectCI->setASTContext(AST.release());
+ AddExternalSource(*IndirectCI, CI);
+ return IndirectCI;
+}
+
llvm::Error ParseSource(const std::string , CompilerInstance ,
CodeGenerator ) {
SourceManager = CI.getSourceManager();
@@ -231,7 +258,9 @@
std::unique_ptr AST =
init_convenience::BuildASTContext(*CI, *ST, *BC);
CI->setASTContext(AST.release());
- AddExternalSource(*CI, Imports);
+ if (Imports.size()) {
+AddExternalSource(*CI, Imports);
+ }
auto LLVMCtx = llvm::make_unique();
std::unique_ptr CG =
@@ -268,12 +297,26 @@
ImportCIs.push_back(std::move(*ImportCI));
}
}
+ std::vector IndirectCIs;
+ if (!Direct) {
+for (auto : ImportCIs) {
+ llvm::Expected IndirectCI =
+ BuildIndirect(ImportCI);
+ if (auto E = IndirectCI.takeError()) {
+llvm::errs() << llvm::toString(std::move(E));
+exit(-1);
+ } else {
+IndirectCIs.push_back(std::move(*IndirectCI));
+ }
+}
+ }
llvm::Expected ExpressionCI =
- Parse(Expression, ImportCIs);
+ Parse(Expression, Direct ? ImportCIs : IndirectCIs);
if (auto E = ExpressionCI.takeError()) {
llvm::errs() << llvm::toString(std::move(E));
exit(-1);
} else {
return 0;
}
}
+
Index: test/Import/conflicting-struct/test.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/test.cpp
@@ -0,0 +1,7 @@
+// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+void expr() {
+ S MyS;
+ T MyT;
+ MyS.a = 3;
+ MyT.u.b = 2;
+}
Index: test/Import/conflicting-struct/Inputs/S2.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/Inputs/S2.cpp
@@ -0,0 +1,7 @@
+class U {
+ int b;
+};
+
+class T {
+ U u;
+};
Index: test/Import/conflicting-struct/Inputs/S1.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/Inputs/S1.cpp
@@ -0,0 +1,6 @@
+class T;
+
+class S {
+ T *t;
+ int a;
+};
Index: lib/AST/ExternalASTMerger.cpp
===
--- lib/AST/ExternalASTMerger.cpp
+++ lib/AST/ExternalASTMerger.cpp
@@ -178,3 +178,9 @@
}
});
}
+
+void ExternalASTMerger::CompleteType(TagDecl *Tag) {
+ SmallVector Result;
+ FindExternalLexicalDecls(Tag, [](Decl::Kind) { return true; }, Result);
+ Tag->setHasExternalLexicalStorage(false);
+}
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Looks good!
Comment at: lib/AST/ASTImporter.cpp:1757
D2->setAnonymousStructOrUnion(true);
+if (PrevDecl) {
+ D2->setPreviousDecl(PrevDecl);
By the way, should we do the same for some other kinds of Decls (like
FunctionDecl)? If so, could you write a NOTE comment?
Repository:
rL LLVM
https://reviews.llvm.org/D32981
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
spyffe updated this revision to Diff 98218.
spyffe added a comment.
Eliminated a //missing newline at end of file// error
Repository:
rL LLVM
https://reviews.llvm.org/D32981
Files:
include/clang/AST/ExternalASTMerger.h
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
test/Import/conflicting-struct/Inputs/S1.cpp
test/Import/conflicting-struct/Inputs/S2.cpp
test/Import/conflicting-struct/test.cpp
tools/clang-import-test/clang-import-test.cpp
Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -42,6 +42,10 @@
Imports("import", llvm::cl::ZeroOrMore,
llvm::cl::desc("Path to a file containing declarations to import"));
+static llvm::cl::opt
+Direct("direct", llvm::cl::Optional,
+ llvm::cl::desc("Use the parsed declarations without indirection"));
+
static llvm::cl::list
ClangArgs("Xcc", llvm::cl::ZeroOrMore,
llvm::cl::desc("Argument to pass to the CompilerInvocation"),
@@ -172,6 +176,14 @@
return Ins;
}
+std::unique_ptr
+BuildCompilerInstance(ArrayRef ClangArgs) {
+ std::vector ClangArgv(ClangArgs.size());
+ std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); });
+ return init_convenience::BuildCompilerInstance(ClangArgv);
+}
+
std::unique_ptr
BuildASTContext(CompilerInstance , SelectorTable , Builtin::Context ) {
auto AST = llvm::make_unique(
@@ -205,6 +217,21 @@
CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
}
+std::unique_ptr BuildIndirect(std::unique_ptr ) {
+ std::vector ClangArgv(ClangArgs.size());
+ std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); });
+ std::unique_ptr IndirectCI =
+ init_convenience::BuildCompilerInstance(ClangArgv);
+ auto ST = llvm::make_unique();
+ auto BC = llvm::make_unique();
+ std::unique_ptr AST =
+ init_convenience::BuildASTContext(*IndirectCI, *ST, *BC);
+ IndirectCI->setASTContext(AST.release());
+ AddExternalSource(*IndirectCI, CI);
+ return IndirectCI;
+}
+
llvm::Error ParseSource(const std::string , CompilerInstance ,
CodeGenerator ) {
SourceManager = CI.getSourceManager();
@@ -231,7 +258,9 @@
std::unique_ptr AST =
init_convenience::BuildASTContext(*CI, *ST, *BC);
CI->setASTContext(AST.release());
- AddExternalSource(*CI, Imports);
+ if (Imports.size()) {
+AddExternalSource(*CI, Imports);
+ }
auto LLVMCtx = llvm::make_unique();
std::unique_ptr CG =
@@ -268,12 +297,26 @@
ImportCIs.push_back(std::move(*ImportCI));
}
}
+ std::vector IndirectCIs;
+ if (!Direct) {
+for (auto : ImportCIs) {
+ llvm::Expected IndirectCI =
+ BuildIndirect(ImportCI);
+ if (auto E = IndirectCI.takeError()) {
+llvm::errs() << llvm::toString(std::move(E));
+exit(-1);
+ } else {
+IndirectCIs.push_back(std::move(*IndirectCI));
+ }
+}
+ }
llvm::Expected ExpressionCI =
- Parse(Expression, ImportCIs);
+ Parse(Expression, Direct ? ImportCIs : IndirectCIs);
if (auto E = ExpressionCI.takeError()) {
llvm::errs() << llvm::toString(std::move(E));
exit(-1);
} else {
return 0;
}
}
+
Index: test/Import/conflicting-struct/test.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/test.cpp
@@ -0,0 +1,7 @@
+// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+void expr() {
+ S MyS;
+ T MyT;
+ MyS.a = 3;
+ MyT.u.b = 2;
+}
Index: test/Import/conflicting-struct/Inputs/S2.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/Inputs/S2.cpp
@@ -0,0 +1,7 @@
+class U {
+ int b;
+};
+
+class T {
+ U u;
+};
Index: test/Import/conflicting-struct/Inputs/S1.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/Inputs/S1.cpp
@@ -0,0 +1,6 @@
+class T;
+
+class S {
+ T *t;
+ int a;
+};
Index: lib/AST/ExternalASTMerger.cpp
===
--- lib/AST/ExternalASTMerger.cpp
+++ lib/AST/ExternalASTMerger.cpp
@@ -178,3 +178,9 @@
}
});
}
+
+void ExternalASTMerger::CompleteType(TagDecl *Tag) {
+ SmallVector Result;
+ FindExternalLexicalDecls(Tag, [](Decl::Kind) { return true; }, Result);
+ Tag->setHasExternalLexicalStorage(false);
+}
Index: lib/AST/ASTStructuralEquivalence.cpp
===
---
[PATCH] D32981: [ASTImporter] Improve handling of incomplete types
spyffe created this revision.
`ASTImporter` has some bugs when it's importing types that themselves come from
an `ExternalASTSource`. This is exposed particularly in the behavior when
comparing complete TagDecls with forward declarations. This patch does several
things:
- Adds a test case making sure that conflicting forward-declarations are
resolved correctly;
- Extends the `clang-import-test` harness to test two-level importing, so that
we make sure we complete types when necessary; and
- Fixes a few bugs I found this way. Failure to complete types was one;
however, I also discovered that complete RecordDecls aren't properly added to
the `redecls` chain for existing forward declarations.
Repository:
rL LLVM
https://reviews.llvm.org/D32981
Files:
include/clang/AST/ExternalASTMerger.h
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
test/Import/conflicting-struct/Inputs/S1.cpp
test/Import/conflicting-struct/Inputs/S2.cpp
test/Import/conflicting-struct/test.cpp
tools/clang-import-test/clang-import-test.cpp
Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -42,6 +42,10 @@
Imports("import", llvm::cl::ZeroOrMore,
llvm::cl::desc("Path to a file containing declarations to import"));
+static llvm::cl::opt
+Direct("direct", llvm::cl::Optional,
+ llvm::cl::desc("Use the parsed declarations without indirection"));
+
static llvm::cl::list
ClangArgs("Xcc", llvm::cl::ZeroOrMore,
llvm::cl::desc("Argument to pass to the CompilerInvocation"),
@@ -172,6 +176,14 @@
return Ins;
}
+std::unique_ptr
+BuildCompilerInstance(ArrayRef ClangArgs) {
+ std::vector ClangArgv(ClangArgs.size());
+ std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); });
+ return init_convenience::BuildCompilerInstance(ClangArgv);
+}
+
std::unique_ptr
BuildASTContext(CompilerInstance , SelectorTable , Builtin::Context ) {
auto AST = llvm::make_unique(
@@ -205,6 +217,21 @@
CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
}
+std::unique_ptr BuildIndirect(std::unique_ptr ) {
+ std::vector ClangArgv(ClangArgs.size());
+ std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); });
+ std::unique_ptr IndirectCI =
+ init_convenience::BuildCompilerInstance(ClangArgv);
+ auto ST = llvm::make_unique();
+ auto BC = llvm::make_unique();
+ std::unique_ptr AST =
+ init_convenience::BuildASTContext(*IndirectCI, *ST, *BC);
+ IndirectCI->setASTContext(AST.release());
+ AddExternalSource(*IndirectCI, CI);
+ return IndirectCI;
+}
+
llvm::Error ParseSource(const std::string , CompilerInstance ,
CodeGenerator ) {
SourceManager = CI.getSourceManager();
@@ -231,7 +258,9 @@
std::unique_ptr AST =
init_convenience::BuildASTContext(*CI, *ST, *BC);
CI->setASTContext(AST.release());
- AddExternalSource(*CI, Imports);
+ if (Imports.size()) {
+AddExternalSource(*CI, Imports);
+ }
auto LLVMCtx = llvm::make_unique();
std::unique_ptr CG =
@@ -268,12 +297,26 @@
ImportCIs.push_back(std::move(*ImportCI));
}
}
+ std::vector IndirectCIs;
+ if (!Direct) {
+for (auto : ImportCIs) {
+ llvm::Expected IndirectCI =
+ BuildIndirect(ImportCI);
+ if (auto E = IndirectCI.takeError()) {
+llvm::errs() << llvm::toString(std::move(E));
+exit(-1);
+ } else {
+IndirectCIs.push_back(std::move(*IndirectCI));
+ }
+}
+ }
llvm::Expected ExpressionCI =
- Parse(Expression, ImportCIs);
+ Parse(Expression, Direct ? ImportCIs : IndirectCIs);
if (auto E = ExpressionCI.takeError()) {
llvm::errs() << llvm::toString(std::move(E));
exit(-1);
} else {
return 0;
}
}
+
Index: test/Import/conflicting-struct/test.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/test.cpp
@@ -0,0 +1,7 @@
+// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+void expr() {
+ S MyS;
+ T MyT;
+ MyS.a = 3;
+ MyT.u.b = 2;
+}
Index: test/Import/conflicting-struct/Inputs/S2.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/Inputs/S2.cpp
@@ -0,0 +1,7 @@
+class U {
+ int b;
+};
+
+class T {
+ U u;
+};
Index: test/Import/conflicting-struct/Inputs/S1.cpp
===
--- /dev/null
+++ test/Import/conflicting-struct/Inputs/S1.cpp
@@ -0,0 +1,6 @@
