Bug 134798 – [iOS][WK2] Fix withinEpsilon()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[iOS][WK2] Fix withinEpsilon()

[Benjamin Poulain]

Created attachment 234691 [details]
Patch

[Benjamin Poulain]

Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers.

[zalan]

(In reply to comment #2)
> Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers.

Oh, probably Darin wants to look at it too.

[Darin Adler]

(In reply to comment #2)
> Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers.

Not sure where you looked. I found this code in /usr/include/c++/4.2.1/cmath:

  inline double
  abs(double __x)
  { return __builtin_fabs(__x); }

  inline float
  abs(float __x)
  { return __builtin_fabsf(__x); }

  inline long double
  abs(long double __x)
  { return __builtin_fabsl(__x); }

[Darin Adler]

Comment on attachment 234691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234691&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:792
> +    return fabs(a - b) < std::numeric_limits<float>::epsilon();

We definitely don’t want fabs when the argument is a float. If you can’t use std::abs for this (I absolutely think you can), you can overload yourself:

    inline double overloadedAbs(double number) { return fabs(number); }
    inline float overloadedAbs(float number) { return fabsf(number); }

    return overloadedAbs(a - b) < std::numeric_limits<float>::epsilon();

Also, I think this needs a comment explaining why we use the float epsilon rather than the epsilon of the type of the return value of fabs. The use of the float epsilon even when comparing double values is something we are doing intentionally for a good reason, but a non-obvious technique that needs a why comment.

[Benjamin Poulain]

(In reply to comment #4)
> (In reply to comment #2)
> > Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers.
> 
> Not sure where you looked. I found this code in /usr/include/c++/4.2.1/cmath:

I used Xcode to follow the symbol std::abs(double) and it sent me to stdlib... :(

I'll update the patch.

[Darin Adler]

(In reply to comment #6)
> I used Xcode to follow the symbol std::abs(double) and it sent me to stdlib... :(

You should search for std::abs elsewhere in WebKit, then. I’m pretty sure that David Kilzer landed a patch that uses std::abs on floating point numbers recently, and if it’s truncating to integers, it would be good to know!

Maybe it’s depending on whether we include <cmath>?

[Benjamin Poulain]

Created attachment 234729 [details]
Patch

[Benjamin Poulain]

(In reply to comment #7)
> (In reply to comment #6)
> > I used Xcode to follow the symbol std::abs(double) and it sent me to stdlib... :(
> 
> You should search for std::abs elsewhere in WebKit, then. I’m pretty sure that David Kilzer landed a patch that uses std::abs on floating point numbers recently, and if it’s truncating to integers, it would be good to know!
> 
> Maybe it’s depending on whether we include <cmath>?

I changed to std::abs(), then inspected the assembly: everything is done with floating point. It is probably just Xcode not resolving the right symbol.

[Darin Adler]

Comment on attachment 234729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234729&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:792
> +    return std::abs(a - b) < std::numeric_limits<float>::epsilon();

I still think a comment is needed here to explain why it’s float epsilon rather than the epsilon for the type of the result of a - b.

[Benjamin Poulain]

Committed r171078: <http://trac.webkit.org/changeset/171078>