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. >> 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. Thanks, Derek > Greetings > bu5hm4n > > >> >> regards >> Stefan Schmidt >> >> ------------------------------------------------------------------------------ >> 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 > > ------------------------------------------------------------------------------ > 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 > ------------------------------------------------------------------------------ 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
