flx marked an inline comment as done. flx 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); ---------------- 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? 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