[PATCH] D103021: [clang-tidy] performance-unnecessary-copy-initialization: Search whole function body for variable initializations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:62
 
 AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) {
   auto OldVarDeclRef =
----------------
This name seems a little off now. maybe rename?


================
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:
> > flx wrote:
> > > ymandel wrote:
> > > > 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.
> > > I was not able to pass the (possibly more narrow) DeclContext to the match function as scope since match does not support DeclContexts.
> > > 
> > > I implemented  isDeclaredInFunction() which iterates through the decl contexts as you suggested. I'm not sure though whether we should start with VarDecl::getDeclContext() or VarDecl::getLexicalDeclContext()?
> > > 
> > > While looking at VarDecl::getLexicalDeclContext() I discovered is VarDecl has the following method:
> > > 
> > > ```
> > >   /// Returns true for local variable declarations other than parameters.                                                    
> > >   /// Note that this includes static variables inside of functions. It also                                                  
> > >   /// includes variables inside blocks.                                                                                      
> > >   ///                                                                                                                        
> > >   ///   void foo() { int x; static int y; extern int z; }                                                                    
> > >   bool isLocalVarDecl() const;
> > > ```
> > > 
> > > I think this is exactly what we'd want here. What do you think?
> > > 
> > You mean instead of `isDeclaredInFunction`?  If so -- yes, that seems right.  But, if so, are you still planning to bind "declStmt" with the `match`, or will you replace that with something more direct?
> I was able to examine the`VarDecl.getInit()` expression directly. This completely avoids a search inside the function and he FunctionDecl is also no longer needed. PTAL.
Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103021