On Wed, Dec 21, 2016 at 11:02:27AM -0600, Derek Foreman 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.
Okay, then you need to know the code to resolve the issues. Anyway your example just showed that those things requires to contact the devs which have worked on that code in the past, and that it is not a good idea to enable it and see who will fix it. > >> 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? Why? This makes absolutly no sense. Why should i revert a patch locally so i have clean compiler output again? There is no point in having them in the master as long as they are not resolved. I am not interested in any of those warnings when i am writing on my code, i have not caused them, why should i resolve them now? > Floating point comparison in C leads to very difficult to diagnose bugs. > > I'm strongly in favor of turning this warning back on immediately. You can, if they are resolved. > 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 ------------------------------------------------------------------------------ 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
