Summary: | [iOS][WK2] Fix withinEpsilon() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, zalan | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Benjamin Poulain
2014-07-09 22:44:14 PDT
Created attachment 234691 [details]
Patch
Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers. (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. (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 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. (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. (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>? Created attachment 234729 [details]
Patch
(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 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. Committed r171078: <http://trac.webkit.org/changeset/171078> |