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

[Cédric Venet]

Le 12/11/2009 22:51, Chris Lattner a écrit :
>
> 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'.
>
> 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?
>
>    

Myself, I would prefer no fixit in this case as it is ambiguous. I 
certainly do not want the fixit:

i&  (1 == 0)

the other one would be the solution in a lot more cases I think, but 
changing the the semantic can be dangerous in case of "tricky" code. If 
we want developper to use the autofixit, we need to make only obviuous, 
non ambiguous fix. (else we could be hiding horrible bugs)

just my 2cents