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

[Sean Callanan via cfe-commits]

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/96d8b9a1/attachment.html>