Page MenuHomePhabricator

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

Authored by flx on May 24 2021, 6:50 AM.

Details

Summary

This fixes false positive cases where a reference is initialized outside of a
block statement and then its initializing variable is modified. Another case is
when the looped over container is modified.

Diff Detail

Event Timeline

flx created this revision.May 24 2021, 6:50 AM
flx requested review of this revision.May 24 2021, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2021, 6:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ymandel added inline comments.Jun 2 2021, 7:28 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
109–110

Consider inspecting the DeclContexts 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;
111

Maybe a bit clearer:
// Search the whole function body, not just the current block statement, for decl statements of the initialization variable.

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
611

typo: Lambda

flx updated this revision to Diff 350994.Jun 9 2021, 2:13 PM
flx marked 2 inline comments as done.

Addressed first round of comments.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
109–110

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.

ymandel added inline comments.Jun 10 2021, 12:08 PM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
109–110

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.

flx updated this revision to Diff 351425.Jun 11 2021, 5:55 AM
flx marked an inline comment as not done.

Use more efficient method to check for local variable declaration.

flx marked an inline comment as done.Jun 11 2021, 6:00 AM
flx added inline comments.
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
109–110

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?

ymandel added inline comments.Jun 14 2021, 9:37 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
109–110

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?

flx updated this revision to Diff 352233.Jun 15 2021, 1:34 PM
flx marked an inline comment as done.

Directly examine initializer.

flx updated this revision to Diff 352236.Jun 15 2021, 1:37 PM

Remove now unnecessary FunctionDecl.

flx marked an inline comment as done.Jun 15 2021, 1:40 PM
flx added inline comments.
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
109–110

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.

ymandel added inline comments.Jun 16 2021, 1:33 PM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
76

This name seems a little off now. maybe rename?

109–110

Nice!

ymandel accepted this revision.Jun 16 2021, 1:33 PM
This revision is now accepted and ready to land.Jun 16 2021, 1:33 PM
flx updated this revision to Diff 353064.Jun 18 2021, 11:26 AM
flx marked 2 inline comments as done.

Renamed initializer matcher.

This revision was landed with ongoing or failed builds.Jun 18 2021, 12:27 PM
This revision was automatically updated to reflect the committed changes.