On Wed, Dec 21, 2016 at 9:13 AM, Gustavo Sverzut Barbieri <[email protected]> wrote: > On Wed, Dec 21, 2016 at 3:02 PM, Derek Foreman <[email protected]> wrote: >> On 21/12/16 10:45 AM, [email protected] wrote: >>> On Wed, Dec 21, 2016 at 05:33:20PM +0100, Stefan Schmidt wrote: >>>> On 21/12/16 17:02, Mike Blumenkrantz wrote: >>>>> On Wed, Dec 21, 2016 at 5:11 AM Stefan Schmidt <[email protected]> >>>>> wrote: >>>>>> On 20/12/16 17:36, Mike Blumenkrantz wrote: >>>>> 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.
I think it is also my bad. I have only introduced a function to compare number, while we do have quite a few comparison against zero. I am investigating using fpclassify, but it seems portable enough as a solution. If so, I will introduce another macro just to tests against zero. I know this doesn't apply to that particular piece of code, as they are integer in this case. >>>> 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 The amount of warning to fix is large and won't ever be fixed I believe too if it isn't in the face of everyone. Now we can delay that for the stabilization period, but putting under the rug and expecting it to be magically fixed at some point in the futur is not something I believe in. ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
