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

[Sean Callanan via cfe-commits]

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 <scallanan at 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 <reviews at reviews.llvm.org <mailto:reviews at 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>
>> 
>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170306/bbe493af/attachment-0001.html>