Bug 134798 - [iOS][WK2] Fix withinEpsilon()
Summary: [iOS][WK2] Fix withinEpsilon()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-09 22:44 PDT by Benjamin Poulain
Modified: 2014-07-14 14:02 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2014-07-09 22:51 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (2.88 KB, patch)
2014-07-10 16:19 PDT, Benjamin Poulain
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-07-09 22:44:14 PDT
[iOS][WK2] Fix withinEpsilon()
Comment 1 Benjamin Poulain 2014-07-09 22:51:58 PDT
Created attachment 234691 [details]
Patch
Comment 2 Benjamin Poulain 2014-07-09 22:56:26 PDT
Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers.
Comment 3 zalan 2014-07-10 08:15:32 PDT
(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.
Comment 4 Darin Adler 2014-07-10 09:45:02 PDT
(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); }
Comment 5 Darin Adler 2014-07-10 09:47:47 PDT
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.
Comment 6 Benjamin Poulain 2014-07-10 13:57:07 PDT
(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.
Comment 7 Darin Adler 2014-07-10 15:51:53 PDT
(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>?
Comment 8 Benjamin Poulain 2014-07-10 16:19:54 PDT
Created attachment 234729 [details]
Patch
Comment 9 Benjamin Poulain 2014-07-10 16:22:44 PDT
(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.
Comment 10 Darin Adler 2014-07-10 17:39:16 PDT
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.
Comment 11 Benjamin Poulain 2014-07-14 14:02:49 PDT
Committed r171078: <http://trac.webkit.org/changeset/171078>