ymandel added inline comments.

================
Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:98-101
   auto Matches =
       match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar))))
                         .bind("declStmt")),
+            Body, Context);
----------------
flx wrote:
> ymandel wrote:
> > Consider inspecting the `DeclContext`s instead, which should be much more 
> > efficient than searching the entire block.  Pass the `FunctionDecl` as an 
> > argument instead of `Body`, since it is a `DeclContext`.  e.g. `const 
> > DeclContext &Fun`
> > 
> > Then, either
> > 1. Call `Fun.containsDecl(InitializingVar)`, or
> > 2. Search through the contexts yourself; something like:
> > ```
> > DeclContext* DC = InitializingVar->getDeclContext(); 
> > while (DC != nullptr && DC != &Fun)
> >   DC = DC->getLexicalParent();
> > if (DC == nullptr)
> >   // The reference or pointer is not initialized anywhere witin the 
> > function. We
> >   // assume its pointee is not modified then.
> >   return true;
> > ```
> Are #1 and #2 equivalent? From the implementation and comment I cannot tell 
> whether #1 would cover cases where the variable is not declared directly in 
> the function, but in child block:
> 
> ```
> void Fun() {
>  {
>    var i;
>    {
>      i.usedHere();
>    }  
>  } 
> }
> ```
> 
> I'm also reading this as an optimization to more quickly determine whether we 
> can stop here. We still need to find the matches for the next steps, but I  
> think I could then limit matching to the DeclContext I found here. Is this 
> correct? For this I would actually need the DeclContext result from #2.
A. I think you're right that #2 is more suited to what you need. I wasn't sure, 
so included both. Agreed that the comments are ambiguous.
B. yes, this is just an optimization. it may be premature for that matter; just 
that match can be expensive and this seemed a more direct expression of the 
algorithm.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103021/new/

https://reviews.llvm.org/D103021

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to