[cfe-commits] PATCH: Adds -Wself-assign to Clang, but defaults it to off

[Douglas Gregor]

On Jan 4, 2011, at 1:28 AM, Chandler Carruth wrote:

> On Mon, Jan 3, 2011 at 11:25 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, Jan 3, 2011 at 10:57 PM, Chandler Carruth <chandlerc at google.com> wrote:
> Perhaps we could add this note as a follow-on when an expression statement consists of just a self-assignment?
> 
> This is a good idea about how to key the fix-it hint. I'll submit this as a follow-up, and then run it over LLVM and Clang, as it seems we aren't immune to this code pattern. =D
> 
> I'm finding this a bit ugly to implement... Currently, the diagnostic is nicely isolated in the expression building part of Sema.  This is nice because it can catch oddities like f(x = x);, but I don't see a nice way to check that this is the only component of the statement. We could just emit the notes from a separate routine in SemaStmt.cpp, and give it the same location? Is that the best way to do this?
> 
> Here is what I ended up with. Not sure if this is the right way forward, or there are other ways to approach matters. I have some orthogonal cleanups queued as well, but I'll see how this shapes up first.

Hrm, this only works correctly when the self-assignment warning was the last warning emitted before we make the callback to specify that the expression is an expression statement. Even if we manage to satisfy that property now, it's going to be hard to guarantee that this property holds in the future. Sorry, I think I led you astray with my "follow-on" comment.

The only feasible idea I have at the moment is a bit of work: add an ActOnSubExpr callback that the parser (and tree transformer) invokes whenever a given expression is used as a subexpression. That way, all expressions either go through ActOnSubExpr or ActOnFinishFullExpr. Both routines would perform the self-assignment check, but the latter would also emit the note (it may need to be extended with some kind of "this result is completely ignored" flag). 

Now, I admit that I have other motives :) I actually found a different need for ActOnSubExpr last week, in the context of some experimental decltype hacking. It was relatively easy to add ActOnSubExpr calls into the tree transformer (where I needed them), but I hadn't pushed them into the parser. In any case, I think Clang would be better for it: we have something similar for declarations, where we know that we'll get one of {AddInitializerToDecl, ActOnInitializerError, ActOnUninitializedDecl}.

> As a side note, the textual display for this hint is... terrible. =/ I'm wondering if we should look at a richer text display lest these diagnostics begin to hinder rather than help outside of an IDE.

We're pretty limited in what we can do in the terminal, but I'm open to ideas!

  - Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110104/74647b54/attachment.html>