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:
> > > 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.
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
