Rightpoint / RZUtils

Rightpoint Commonly Used Tools
Other
123 stars 22 forks source link

Use fabs instead of fabsf to manipulate CGFloat values. #152

Closed ZevEisenberg closed 9 years ago

ZevEisenberg commented 9 years ago

Silences a compiler warning.

mr-fixit commented 9 years ago

I asked on Slack if anyone knew a reason I shouldn't merge this. Tasaki and I exchanged texts and emails, I'm paraphrasing his comment here:

T: Because precision is lost from float to double* and the originally intended comparison may no longer work.

*[originally he said it the other way, but corrected himself]

Then he expanded:

In such conversion, a precision is not maintained:
float x = 250.840;
double y = 0.0;
y = x;
NSLog(@"%f", y); //Prints 250.83996

Float fits in double obviously because double is a bigger type, fits float in bit, 
but no longer represents the same number.  For a more accurate 
comparison setting epsilon and checking for a threshold is preferred method.

There is no conversion that takes place explicitly in the pull request's code 
but what happens when the code runs on 32-bit?

#if defined(__LP64__) && __LP64__
# define CGFLOAT_TYPE double
# define CGFLOAT_IS_DOUBLE 1
# define CGFLOAT_MIN DBL_MIN
# define CGFLOAT_MAX DBL_MAX
#else
# define CGFLOAT_TYPE float
# define CGFLOAT_IS_DOUBLE 0
# define CGFLOAT_MIN FLT_MIN
# define CGFLOAT_MAX FLT_MAX
#endif

/* Definition of the `CGFloat' type and 'CGFLOAT_DEFINED'. */
typedef CGFLOAT_TYPE CGFloat;

CGFloat is deceptive on how it is declared.  When we target 64bit 
unlike what its name implies it's really a double.  Compiler I think 
was showing warning with fabsf() because of this.

What happens if the pull request code runs on 32-bit environment when
 CGFloat is not a double but a float? Float is passed to fabs() so the 
parameter is promoted to double.  Consider this method:

- (void) foo:(double)myDouble
{
    NSLog(@"myDouble is %f", myDouble);
}

float x = 250.840;
[self foo:x]; //Prints myDouble is 250.83996

I expected a similar behavior by fabs() as shown above and thought that
the comparison in the code no longer is comparing the values which the 
code really intends to compare.

Perhaps an enhanced solution could be checking for 32-bit vs 64-bit 
environment and call the appropriate fabs() vs fabsf().

Just my 5cents.

Thanks,

-Takashi
mr-fixit commented 9 years ago

Here are my thoughts on Takashi's points:

1) precision isn't lost when converting from float to double, but the base-10 expression of the value may change.

2) Since the code in question is doing a "less-than" comparison, in a 32-bit environment it will give the same answer across all values regardless of whether fabs() or fabsf() is used. In a 64-bit environment, it is necessary to use fabsf() to get the right answer across the full range of numbers.

3) the line "NSLog(@"%f", y)" should use '%g', not '%f'.

In summary, I still think Zev's code is right and should be merged.

Takashi, please forgive me if I've misunderstood or misrepresented your (very much appreciated) feedback.

ZevEisenberg commented 9 years ago

While I agree that we should be cautious with float precision, in general the best practice is just to use the double versions of the math.h functions. That’s what Apple’s warning in Xcode 6.3 recommends.

In the particular case where it’s used here, both sides of the comparison are being converted the same way, and we’re either maintaining or gaining precision.

rztakashi commented 9 years ago

@ZevEisenberg @mr-fixit "3) the line "NSLog(@"%f", y)" should use '%g', not '%f'." <-- oops! my bad.

"both sides of the comparison are being converted the same way". This is a point that I absolutely agree and lets me believe that this code is rather safe.

+1 for merge from me!