<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Gosh, I missed the reserve() call. Sorry! I'll be happy to implement this change.<div class=""><br class=""></div><div class="">Sean</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 6, 2017, at 10:05 AM, Sean Callanan <<a href="mailto:scallanan@apple.com" class="">scallanan@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Aleksei,<div class=""><br class=""></div><div class="">thank you for your comments!</div><div class=""><br class=""></div><div class=""><ul class="MailOutline"><li class="">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.</li><li class="">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?</li><li class="">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.</li></ul><div class=""><br class=""></div></div><div class="">Sean</div><div class=""><br class=""></div><div class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 6, 2017, at 9:48 AM, Aleksei Sidorin via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">a.sidorin added a comment.<br class=""><br class="">Hello Sean,<br class=""><br class="">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?<br class=""><br class=""><br class=""><br class="">================<br class="">Comment at: tools/clang-import-test/clang-import-test.cpp:201<br class="">+ Decl *Imported(Decl *From, Decl *To) override {<br class="">+ if (auto ToTag = llvm::dyn_cast<TagDecl>(To)) {<br class="">+ ToTag->setHasExternalLexicalStorage();<br class="">----------------<br class="">Can we make the usage of qualified and non-qualified casts consistent? Usually, casts are used as unqualified.<br class=""><br class=""><br class="">================<br class="">Comment at: tools/clang-import-test/clang-import-test.cpp:298<br class="">+<br class="">+ std::vector<Candidate> CompleteDecls;<br class="">+ std::vector<Candidate> ForwardDecls;<br class="">----------------<br class="">I guess we can use `SmallVector`s here too.<br class=""><br class=""><br class="">================<br class="">Comment at: tools/clang-import-test/clang-import-test.cpp:303<br class="">+ if (IsForwardDeclaration(C.first)) {<br class="">+ if (std::none_of(ForwardDecls.begin(), ForwardDecls.end(),<br class="">+ [&C](Candidate &D) {<br class="">----------------<br class="">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.<br class=""><br class=""><br class="">================<br class="">Comment at: tools/clang-import-test/clang-import-test.cpp:333<br class="">+ Decls.resize(DeclsToReport.size());<br class="">+ std::transform(DeclsToReport.begin(), DeclsToReport.end(), Decls.begin(),<br class="">+ [](Candidate &C) {<br class="">----------------<br class="">The loop:<br class="">```<br class="">Decls.reserve(DeclsToReport.size());<br class="">for (Candidate &C : DeclsToReport)<br class=""> Decls.push_back(cast<NamedDecl>(C.second->Import(C.first)));<br class="">```<br class="">will look a bit nicer here. What do you think?<br class=""><br class=""><br class="">Repository:<br class=""> rL LLVM<br class=""><br class=""><a href="https://reviews.llvm.org/D30435" class="">https://reviews.llvm.org/D30435</a><br class=""><br class=""><br class=""><br class=""></div></div></blockquote></div><br class=""></div></div></div></div></blockquote></div><br class=""></div></body></html>