Aleksei, thank you for your comments!
I appreciate your comments in particular about source/destination confusion. I can relate to this, as this was (a) an area that confused me while I was working on this code in LLDB; and (b) it was something I had to keep straight in my head even when doing the work here. I will implement a type-system based solution to this and update the patch, then you can have a look and we'll decide if it looks better that way. The std::transform is more efficient than the for loop because it reallocates the array once at the beginning, instead of progressively as we push_back. This means the asymptotic runtime of this loop will be worse than the std::transform. Is that an acceptable trade? The other points are well-taken – in particular, I like your idea about breaking the std::none_of block out into a function. I'll apply them in an updated patch. Sean > On Mar 6, 2017, at 9:48 AM, Aleksei Sidorin via Phabricator > <revi...@reviews.llvm.org <mailto:revi...@reviews.llvm.org>> wrote: > > 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 <https://reviews.llvm.org/D30435> > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
