Re: [ros-dev] [ros-diffs] [tkreuzer] 63520: [MCISEQ] Silence a warning [CMAKE] Get rid of -Wtype-limits, it's noisy, it doesn't provide any reasonable benefit and it's almost impossible to "fix" these warnings without huge ...

[Thomas Faber]

This helps find logic errors, mostly where the programmer thought the
variable was signed but it's actually unsigned, e.g. if (ret < 0) for a
function that returns (ULONG)-1.

I also don't see how the fix would be complicated in most cases.
(x <= 0) becomes (x == 0), (x < 0) becomes (x == (ULONG)-1) or whatever
(after a logic review).
I know that it'll be problematic if resulting from a macro... but I
think it's worth finding a workaround for those cases.
Most of the warnings I remember from our code base though (especially
ntoskrnl) didn't involve macros and were actually cases of questionable
logic... although maybe I recall incorrectly.


On 2014-05-31 23:26, tkreuzer(a)svn.reactos.org wrote:
>   [CMAKE] Get rid of -Wtype-limits, it's noisy, it
> doesn't provide any reasonable benefit and it's almost impossible to
> "fix" these warnings without huge haxxory.

[Thomas Faber]

Well I've been watching their number regularly and looked at the list
when it changed ;)

But yeah I agree with you that having zero warnings is much better.
Nice work on that btw.

I had looked through the list of type-limits warnings when I enabled it,
but many required a better understanding of the code in question than I
had without long investigation. I'll keep them in mind for the future,
and will see if I can get rid of them -- my main concern with disabling
the option is that we'll easily introduce the warning in new code.


On 2014-06-14 14:32, Timo Kreuzer wrote:
>   This is also stuff like
>  if (Option < MinOption || Option > MaxOption) return FALSE;
>  where Option is unsigned and MinOption is 0
>  yes, you can remove it, but it's not beneficial for code readability
>  some cases are due to the use of macros.
>  anyway: if you think its's easily fixed, go ahead, fix it and add the option back :)
>  I just don't want to have ANY warnings at all. Once you have a few warnings here 
>  and there, nobody will give a shit anymore and things can easily sneak in 
>  without being noticed. Or are you going through the list of warnings after every 
>  commit?
>  *Gesendet:* Samstag, 14. Juni 2014 um 12:49 Uhr
>  *Von:* "Thomas Faber" &lt;thomas.faber(a)reactos.org&gt;
>  *An:* ros-dev(a)reactos.org
>  *Betreff:* Re: [ros-dev] [ros-diffs] [tkreuzer] 63520: [MCISEQ] Silence a 
>  warning [CMAKE] Get rid of -Wtype-limits, it's noisy, it doesn't provide any 
>  reasonable benefit and it's almost impossible to "fix" these warnings
> without 
>  huge ...
>  This helps find logic errors, mostly where the programmer thought the
>  variable was signed but it's actually unsigned, e.g. if (ret < 0) for a
>  function that returns (ULONG)-1.
>  
>  I also don't see how the fix would be complicated in most cases.
>  (x <= 0) becomes (x == 0), (x < 0) becomes (x == (ULONG)-1) or whatever
>  (after a logic review).
>  I know that it'll be problematic if resulting from a macro... but I
>  think it's worth finding a workaround for those cases.
>  Most of the warnings I remember from our code base though (especially
>  ntoskrnl) didn't involve macros and were actually cases of questionable
>  logic... although maybe I recall incorrectly.
>  
>  
>  On 2014-05-31 23:26, tkreuzer(a)svn.reactos.org wrote:
> >   [CMAKE] Get rid of -Wtype-limits, it's noisy,
> > it doesn't provide any  
>  reasonable benefit and it's almost impossible to
> "fix" these warnings without 
>  huge haxxory.
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>  
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>