On Wed, 21 Dec 2016 16:02:50 +0000 Mike Blumenkrantz <michael.blumenkra...@gmail.com> said:
time to butt in i guess... > > 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; it's not like anyone here is taking the time to review all > these commits once they hit the repo. that's not the issue here. it's that people had other work to do and now they drop it to fix new warnings that were not there before, many of which are just plain noise you cannot shut up without making the code actually worse. if (val == 0.0) is not invalid with floats/doubles if your intent is to actually compare to 0. it can be exact. i saw at least several of these stream past (patches fixing comparisons to 0). but now we have to fix them with less accurate code that is if (val >= 0.0 - x && val <= 0.0 + x) effectively wrapped in a macro which is actually, not the same code. let me start looking for "wrongs": + if (pr->action == EDJE_ACTION_TYPE_STATE_SET && (EQ(pr->tween.time, ZERO) || (ed->no_anim))) + if (EQ(rp->drag->val.x, FROM_DOUBLE(d))) return EINA_TRUE; + if (EQ(v, FROM_INT(1))) ea->delete_me = 1; + if (EQ(sc, ZERO)) sc = DIV(_edje_scale, en->ed->file->base_scale); + if (EQ(sc, ZERO)) sc = DIV(_edje_scale, ed->file->base_scale); + (EQ(ep->typedata.text->cache.align_x, params->type.text->align.x)) && + (EQ(ep->typedata.text->cache.align_y, params->type.text->align.y)) && + (EQ(ep->typedata.text->cache.ellipsis, params->type.text->ellipsis)) && + if (!EINA_DBL_CMP(tag->font_size, 0)) that's just in a single commit. in fact that is every single change in the commit just to shut warnings up. every single one actually was originally totally correct. they now are slower having to first fabs() then do the compare. every single one. the comparisons are either to 0 which is perfectly comparable in their siutaiton. or to 1 WHEN the result is clamped to 1 beforehand with something like if (val > 1) val = 1 which will make it work perfectly, or it's a comparison of "is the current condition the same as the previous or the source when double values will literally be copied from one to the other" and now it's "well are they almost the same" which is NOT the same thing. this warning i would consider harmful. i can go on about commit after commit "fixing warnings" which are not actual issues, and the fixes are actually CHANGING behaviour. now variables that used to get to 0 won't get to 0 anymore for example. values that should have hit 1 will not (well 1.000000000000000002343 or whatever)... and they now may stop at 0.99999999999999999883 ... and when you take these doubles and multiple by an integer... when you used to do int ivar = 255 * val and get 255 (due to rounding down of the just-over-1 value) now you might hit 254 and never get to 255... think about it... this warning is harmful, especially if turned on and the response is "OMG shut the warnings up!", which means it's not practical to turn on. YES. i agree that it MAY spot some errors, BUT looking at everything people are doing to "shut it up" it's actually far far far worse. it's making slower code that doesn't fix any bugs in almost all cases, makes code that is more work to read and maintain and in fact probably is adding more bugs than it fixes now. not every warning flag is a gold mine. i think it's great people fix warnings and issues. i think it's great bugs get fixed. having initiative is great. BUT... this warning flag and what it results in is harmful. it should not be turned on for everyone that then induces some "holy mother of god look at those warnings. i can't see the real issues anymore and have to fix these" response which then fixes nothing and very likely results in adding of bugs. that's the issue here. > > 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. no. i don't think you should. i do think you should be careful of the noise to signal ratio of a warning and the resulting necessary work to "fix it". if you could say "this one is ok" and a an attribute/macro around the comparison that turned off warnings for that one ONCE it has been looked at and someone goes "yeah - that one will work"... then we're looking better. i know of no way to do that off the top of my head. the only way is to CHANGE the code to now no longer do an exact comparison which is more expensive and changing the functionality of the code. > This could have easily been resolved by the end of the week if people had > spent the time making fixes instead of complaining. Furthermore, I think > the fact that the people who complained about the warnings were not the > ones fixing any of them highlights my point. perhaps they believe the warnings are mostly noise/junk and it is a waste of their time and effort to look at them? i sure do. i spent some of my time reading the patches resulting from this and frankly i'm shocked at the commit noise, warning noise etc. - when i first saw the warnings i looked and went "hmm that's ok., that one's ok. that's ok.. so is that. and that... ... i just cant see any issues it finds". it may have found some. i didn't see them with the warnings trawling past. having looked at many of these i'm tempted to say "we need to revert these all and go back to where we were before this debacle and sit down and look at this seriously. are there perhaps a few comparisons that are wrong? indeed there probably are. are there 100's of them that are incorrect and need any changes? hell no there are not. the core issue here is the vast majority of fixes are not fixes. not from what i've seen. the vast majority are "is this value in this state the same as the other" or "is this the same as before" or "is this a known constant like 0, 1 0.5 that get SET to such constants and thus will be an exact match". looking at the patches these changes are even being gotten wrong by people familiar with the code. they are just doing them now to "shut the compiler up". let's stand back. not rush into this and have a decent solution. for the moment that means turning this warning off, as it being on is far more harmful. we need to keep comparisons that are safe as-is. but you need to know that it's safe or not and that means reading the code around the comparison, finding out where the numbers come from etc. ... i LIKE us improving code. one way or another. i applaud people wanting to try and trying. we all need to have a go at this. i normally leave it to "before release" when i start hunting warnings from compilers, coverity and eina err logs. that's what i do. others may do it differently. but some things are more problematic than others to fix and blindly making every floata == floatb into a macro with fabs like above is NOT the correct solution by a long shot and that is exactly what has happened. it is exactly what WILL happen every time a warning flag is added and it creates a mountain of warnings. we are already into the "land of diminishing returns" when it comes to warnings. the warnings that find a bug 50%+ of the time are already dealt with. we're now in "warning finds a bug 1 in 100 times or 1 in 1000 times" land... and we need to be far more careful what warnings are on by default for everyone, and which are not and how we respond to them. please do not get me wrong that these warnings can and did find a bug or 2. it's the collateral damage now that is the issue. -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ 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
