[cfe-commits] r86962 - /cfe/trunk/tools/clang-cc/clang-cc.cpp

[Douglas Gregor]

On Nov 12, 2009, at 1:51 PM, Chris Lattner wrote:

>
> On Nov 12, 2009, at 5:39 AM, Sebastian Redl wrote:
>
>> Daniel Dunbar wrote:
>>> Author: ddunbar
>>> Date: Thu Nov 12 00:48:24 2009
>>> New Revision: 86962
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=86962&view=rev
>>> Log:
>>> clang-cc: -fixit is actually option, not an action, although its
>>> use with non
>>> -fsyntax-only is probably untested.
>>>
>> Also, misleading and not meaningful. Some warnings fixit the code to
>> something else semantically than the current code, e.g. i & 1 == 0 is
>> fixed to (i & 1) == 0. If I do CodeGen+Fixit, I get the old
>> semantics in
>> the object code even while changing the source.
>>
>> So perhaps we should turn -fixit into an action instead of relying on
>> users not to do anything stupid with it.
>
> I think that this is a serious problem with a different fix.  Fixits
> for warnings should be to 'make the warning go away without changing
> the semantics of the code' not 'change my code to make the warning go
> away'.

Agreed.

> In this case, the fixit should insert:
>   i & (1 == 0)
>
> No?  Maybe there should be two modes for fixits: semantic preserving
> ones that can always be safely applied vs potential bugs?


I like the idea of tagging those fix-its that actually change  
behavior. They should not be automatically applied by the -fixit mode.

We could even provide multiple alternatives via different nodes:


	note: to silence this warning, insert paretheses around the ==  
expression

(show 1 == 0 underlined with the fixit adding ( and ) around it. this  
fixit doesn't change behavior)

	note: to evaluate the & first, insert parentheses around the &  
expression

(show i & 1 underlined with the fixit adding ( and ) around it. this  
fixit does change behavior)

	- Doug