a.sidorin added a comment.
Hello Sean,
It is good to have the ability of recursive lookup. But for me (and I'm sorry),
the code becomes very hard to understand: I need to track if the Decl/DC is in
the source AST or in the AST being imported, if it was imported or if was in
the AST before across lambdas and calls. Can we make this more clear?
================
Comment at: tools/clang-import-test/clang-import-test.cpp:201
+ Decl *Imported(Decl *From, Decl *To) override {
+ if (auto ToTag = llvm::dyn_cast<TagDecl>(To)) {
+ ToTag->setHasExternalLexicalStorage();
----------------
Can we make the usage of qualified and non-qualified casts consistent? Usually,
casts are used as unqualified.
================
Comment at: tools/clang-import-test/clang-import-test.cpp:298
+
+ std::vector<Candidate> CompleteDecls;
+ std::vector<Candidate> ForwardDecls;
----------------
I guess we can use `SmallVector`s here too.
================
Comment at: tools/clang-import-test/clang-import-test.cpp:303
+ if (IsForwardDeclaration(C.first)) {
+ if (std::none_of(ForwardDecls.begin(), ForwardDecls.end(),
+ [&C](Candidate &D) {
----------------
Nit: to make the code cleaner, we can extract this `std::none_of` to a separate
function like `IsFoundAsForward()`. Nested lambdas are harder to read.
================
Comment at: tools/clang-import-test/clang-import-test.cpp:333
+ Decls.resize(DeclsToReport.size());
+ std::transform(DeclsToReport.begin(), DeclsToReport.end(), Decls.begin(),
+ [](Candidate &C) {
----------------
The loop:
```
Decls.reserve(DeclsToReport.size());
for (Candidate &C : DeclsToReport)
Decls.push_back(cast<NamedDecl>(C.second->Import(C.first)));
```
will look a bit nicer here. What do you think?
Repository:
rL LLVM
https://reviews.llvm.org/D30435
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
