[cfe-commits] [patch] Warn on self assignment for member variables

[Daniel Jasper]

On Mon, Jun 25, 2012 at 9:42 PM, Nico Weber <thakis at chromium.org> wrote:

> Hi Ted,
>
> On Mon, Jun 25, 2012 at 12:40 PM, Ted Kremenek <kremenek at apple.com> wrote:
> > Hi Nico,
> >
> > How expensive is this check?  This code makes me nervous:
>
> I didn't measure, but note that this is only executed if the lhs and
> rhs are both MemberExprs, and they both refer to the same Decl. This
> isn't true for most assignments.
>

Would it be cheaper to just check whether the MemberExpr directly sets one
member (e.g. var_ = var_)? I think cases like t->s_->a_ = t->s_->a_; are
very rare.

Is it enough to do measure the impact on a full build of chromium, or
> would you like to see more data?
>

The question is whether code with this kind of bug will be commonly
committed to a codebase. My guess would be that this would lead to bugs
that are painstakingly debugged before committing. Thus, the warning might
fire much more often during development. But I don't have data.

Daniel


>
> Nico
>
> >
> > +  llvm::FoldingSetNodeID lhsID, rhsID;
> > +  ML->getBase()->Profile(lhsID, Sema.Context, true);
> > +  MR->getBase()->Profile(rhsID, Sema.Context, true);
> > +  if (lhsID == rhsID)
> > +    Sema.Diag(Loc, diag::warn_identity_memvar_assign);
> >
> > If this is done on every assignment, might this be a bit expensive for
> marginal gain?
> >
> > On Jun 25, 2012, at 12:26 PM, Nico Weber <thakis at chromium.org> wrote:
> >
> >> Hi,
> >>
> >> the attached patch adds a warning for self-assignments of member
> >> variables. This is PR13104. I do this every now and then locally when
> >> writing setter functions: I type "void set_var(int var) { var_ =" and
> >> then try to hit ctrl-p twice to complete the lhs to "var" in vim, but
> >> miss and hit it only once, so that I end up with "var_ = var_;"
> >>
> >> This finds 0 bugs and 0 false positives in chromium, so I'm not sure
> >> how useful this is. Opinions?
> >>
> >> Nico
> >>
> <clang-memvar-assign.patch>_______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120625/ab299927/attachment.html>