On 21/12/16 11:13 AM, Gustavo Sverzut Barbieri wrote: > 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.
Exactly! In this case though: if (pos0->stride == 0) Stride is an integer (both as a generical graphics concept, and in the struct in question). Thanks, Derek > >>>> 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 > > ------------------------------------------------------------------------------ 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
