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

[Douglas Gregor]

On Jan 4, 2011, at 10:28 AM, John McCall wrote:

> On Jan 4, 2011, at 7:41 AM, Douglas Gregor wrote:
>> 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.
> 
> I also can't imagine how we would satisfy that property.
> 
>> 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).
> 
> I don't think this really solves the problem, as a parenthesized expression would clearly be a sub-expression and an initializer would be a full-expression.
> 
> Also, the optimal rewrite for "(a = a);" would probably be "(void) (a);", not "((void) a);".

In my scheme, we just wouldn't end up providing a rewrite for (a = a). I'm okay with that.

> We could have a queue of things to consider at the end of a full-expression.  I think there's even a SourceLocation passed in to MaybeCreateExprWithCleanups which should be invalid iff we're in a ignored context.  On the other hand, I think we don't always get a callback at the end of a broken full-expression.

Sure, that's possible too. I don't much care if we don't emit warnings about a broken full-expression, though.

	- Doug

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