[PATCH] D30435: [clang-import-test] Lookup inside entities

[Aleksei Sidorin via Phabricator via cfe-commits]

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