Gosh, I missed the reserve() call. Sorry! I'll be happy to implement this change.
Sean > On Mar 6, 2017, at 10:05 AM, Sean Callanan <scalla...@apple.com> wrote: > > 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
