On Wed, Dec 21, 2016 at 3:02 PM, Derek Foreman <der...@osg.samsung.com> wrote:
> On 21/12/16 10:45 AM, marcel-hollerb...@t-online.de wrote:
>> On Wed, Dec 21, 2016 at 05:33:20PM +0100, Stefan Schmidt wrote:
>>> Hello.
>>>
>>> On 21/12/16 17:02, Mike Blumenkrantz wrote:
>>>> On Wed, Dec 21, 2016 at 5:11 AM Stefan Schmidt <ste...@osg.samsung.com>
>>>> wrote:
>>>>
>>>>> Hello
>>>>>
>>>>> On 20/12/16 17:36, Mike Blumenkrantz wrote:
>>>>>> I think your "How many real issues have you seen" was the same argument
>>>>>> against running any static analysis a couple years ago; now we have
>>>>> weekly
>>>>>> reports for that.
>>>>>>
>>>>>> I have seen bugs that resulted from illegal float comparison. The fact
>>>>> that
>>>>>> a warning may be a false positive in some or even most cases does not
>>>>>> ensure that every warning is a false positive.
>>>>>>
>>>>>> Given that we are so pedantic about warnings for much more trivial
>>>>> matters
>>>>>> (e.g., -Wunused-parameter as part of -Wextra), it seems bizarre to me
>>>>> that
>>>>>> anyone would complain about enforcing valid comparisons for floats.
>>>>>
>>>>> Getting the code in shape for correct float comparisons is actually
>>>>> something I would like to see.
>>>>>
>>>>> The real problem here is/was how this was handled. Forcing the compiler
>>>>> flag to everyone's build is the problem. That and seeing Chris and
>>>>>
>>>> Cedric going crazy and doing nearly 100 patches in a short timeframe.
>>>>> And even after all this my build was still noisy with all these warnings.
>>>>>
>>>>
>>>> If people want to spend a few hours fixing hundreds of compile warnings
>>>> before leaving on holiday then I think they should be commended, not held
>>>> up as examples of what not to do. There's nothing wrong with creating a lot
>>>> of patches;
>>>
>>> I'm not complaining about the amount of patches, I don't want this fixed
>>> my one big commit either. And if this would have been it and all
>>> warnings fixed I would not reverted anything. Given that Chris even
>>> reverted patches because he did not compile test them before pushing I
>>> really wonder what this rush is about. Is this a battle on getting
>>> patches in before vacation, or what?
>>>
>>> it's not like anyone here is taking the time to review all
>>>> these commits once they hit the repo.
>>>
>>> Just not true. I look over patches coming in over the commit mailing
>>> list. And I know Tom also did. Others I can't say, but neither can you.
>>>
>>>>
>>>>>
>>>>> The reason we are so pedantic with the other warnings from the default
>>>>> flags is that we want to have a clean build output to _actually spot
>>>>> warnings from new code_ we are working on.
>>>>>
>>>>
>>>>> I reverted the patch putting it into the default compiler flags. Enable
>>>>> it locally, get the the amount of warnings down to a sane amount and
>>>>> I'm all for putting it in again. (tests and examples are also full of
>>>>> it. Mentioning it here because I know how much people love to avoid
>>>>> compiling those.)
>>>>>
>>>>
>>>> I'm supposed to enable a flag locally and fix hundreds of warnings by
>>>> myself in code that I've never seen before? Or perhaps you think that
>>>> others will collaborate and do this voluntarily if they have to manually
>>>> enable a warning flag? Please be realistic.
>>>
>>> I am. You could have written a mail highlighting some examples why this
>>> warnings is useful, you could have started a branch with it enabled and
>>> worked together with Chris and Cedric on it to fix the warnings. Nothing
>>> of this happened. Out of the blue this warning was enabled and a rush of
>>> patches came in. You should not be surprised that people bring this up.
>
> Are you asking for an explanation of why floating point comparison with
> == in C is bad? This is axiomatic and hardly needs discussion in this
> forum.
>
>>>> This could have easily been resolved by the end of the week if people had
>>>> spent the time making fixes instead of complaining.
>>>
>>> Interesting that you are not supposed to fix warnings in code that you
>>> have never seen before but others are.
>
> I haven't seen that suggested by anyone, probably because it's a bad idea.
>
>> Little side note: Claiming that you have to know a codebase for changing
>> a==b to EINA_DBL_CMP(a,b) is NOT realistic :)
>
> ENOUGH of this! I've been trying to just sit this one out, but come on!
>
> It is *not* a cookie cutter change! I'm seeing claims that it's
> trivial, and claims the people are reading the commit logs, and then I'm
> seeing patches like this:
>
> ...
> - if (pos0->stride != 0.0)
> + if (!EINA_FLT_CMP(pos0->stride, 0.0))
> ...
>
> Which is obviously wrong if you know what you're looking at.
>
> Further, there may be cases where we intentionally only want a
> comparison to trigger if the value is *exact* (as in, has been set that
> way by assignment). This could be the case in matrix math operations.
>
> These warnings aren't trivially resolved by people with no familiarity
> with the code.
Indeed. A much better approach would be to review with care and even
consider which epsilon is appropriate for that case... it's not a
simple 'sed'... which would just hide problems.
Like in the excerpt above, without knowledge of the usage it's hard to
say if it should be:
if (!EINA_FLT_CMP(pos0->stride, 0.0)) -> unlikely
or:
if (pos->stride > 0.0) -> more likely, but hard to guarantee
without usage knowledge.
>>> Furthermore, I think
>>>> the fact that the people who complained about the warnings were not the
>>>> ones fixing any of them highlights my point.
>>>
>>> Making things up does not help this discussion.
>>
>> Also as I am one of those who complained:
>> I complained because i am working on patches for elementary, i am
>> changing a few focus apis in there. So it is quite meaningfull if a
>> warning comes up, but with searching in 100 Warnings for a single new is not
>> quite handy. Also there is still the thing i have told you in the other
>> mail, why now? And more important, why at all? Is there a issue?
>
> Revert the change locally?
>
> Floating point comparison in C leads to very difficult to diagnose bugs.
>
> I'm strongly in favor of turning this warning back on immediately.
>
> This whole debate makes no sense.
while I find these warnings annoying, I have to agree with this :-D
--
Gustavo Sverzut Barbieri
--------------------------------------
Mobile: +55 (16) 99354-9890
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/intel
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
