Bug 130947 – [iOS] Fix remaining misuses of abs() and fabsf()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Compiling WebKit for iOS with trunk clang shows a few more misuses of abs() and fabsf():

WebCore/platform/ios/wak/WKView.mm:251:30: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value]
                         || (abs(origMarginsTotal) == 1)) {
                             ^
WebCore/platform/ios/wak/WKView.mm:251:30: note: use function 'fabs' instead
                         || (abs(origMarginsTotal) == 1)) {
                             ^~~
                             fabs

WebCore/platform/ios/ScrollAnimatorIOS.mm:125:45: error: using floating point absolute value function 'fabsf' when argument is of integer type [-Werror,-Wabsolute-value]
                    float dragAngle = atanf(fabsf(deltaFromStart.height()) / fabsf(deltaFromStart.width()));
                                            ^
WebCore/platform/ios/ScrollAnimatorIOS.mm:125:45: note: use function 'abs' instead
                    float dragAngle = atanf(fabsf(deltaFromStart.height()) / fabsf(deltaFromStart.width()));
                                            ^~~~~
                                            abs
WebCore/platform/ios/ScrollAnimatorIOS.mm:125:78: error: using floating point absolute value function 'fabsf' when argument is of integer type [-Werror,-Wabsolute-value]
                    float dragAngle = atanf(fabsf(deltaFromStart.height()) / fabsf(deltaFromStart.width()));
                                                                             ^
WebCore/platform/ios/ScrollAnimatorIOS.mm:125:78: note: use function 'abs' instead
                    float dragAngle = atanf(fabsf(deltaFromStart.height()) / fabsf(deltaFromStart.width()));
                                                                             ^~~~~
                                                                             abs
WebCore/platform/ios/ScrollAnimatorIOS.mm:143:114: error: using floating point absolute value function 'fabsf' when argument is of integer type [-Werror,-Wabsolute-value]
        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollLeft : ScrollRight, ScrollByPixel, fabsf(delta));
                                                                                                                 ^
WebCore/platform/ios/ScrollAnimatorIOS.mm:143:114: note: use function 'abs' instead
        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollLeft : ScrollRight, ScrollByPixel, fabsf(delta));
                                                                                                                 ^~~~~
                                                                                                                 abs
WebCore/platform/ios/ScrollAnimatorIOS.mm:149:111: error: using floating point absolute value function 'fabsf' when argument is of integer type [-Werror,-Wabsolute-value]
        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollUp : ScrollDown, ScrollByPixel, fabsf(delta));
                                                                                                              ^
WebCore/platform/ios/ScrollAnimatorIOS.mm:149:111: note: use function 'abs' instead
        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollUp : ScrollDown, ScrollByPixel, fabsf(delta));
                                                                                                              ^~~~~
                                                                                                              abs

[David Kilzer (:ddkilzer)]

Created attachment 228131 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Created an attachment (id=228131) [details]
> Patch v1

Obviously, if this lands with narrowPrecisionTo<>() methods, I would replace functions in Source/WebCore/platform/FloatConversion.h with NumericConversion.h.

[Brent Fulgham]

Comment on attachment 228131 [details]
Patch v1

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

A couple of minor questions, but looks good overall. R=me.

> Source/WTF/wtf/MathExtras.h:395
> +template<> inline double abs(const double& value) { return ::fabs(value); }

Doesn't the existing STL math library (<cmath>, or maybe <cfloat>) provide template methods for these already?

> Source/WTF/wtf/NumericConversion.h:165
> +inline float narrowPrecisionTo(const float& value) { return value; }

Is there any advantage to range checks like you did for double->int here? IIRC, floats have about an order of magnitude smaller range of values they can hold (aside from the obvious precision differences).

> Source/WTF/wtf/NumericConversion.h:168
> +inline float narrowPrecisionTo(const double& value) { return static_cast<float>(value); }

Obviously I meant the previous comment for this line!

[Build Bot]

Comment on attachment 228131 [details]
Patch v1

Attachment 228131 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4882791653179392

New failing tests:
compositing/columns/composited-rl-paginated-repaint.html

[Build Bot]

Created attachment 228134 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[David Kilzer (:ddkilzer)]

Comment on attachment 228131 [details]
Patch v1

Ahh, std::abs() is overloaded to work as expected for floating point types from <cmath>.  I'll use that instead of defining my own macros.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (From update of attachment 228131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228131&action=review
> 
> A couple of minor questions, but looks good overall. R=me.

Thanks!  I want Darin to weigh on this as well because he mentioned he's interested in this topic on Friday.

> > Source/WTF/wtf/MathExtras.h:395
> > +template<> inline double abs(const double& value) { return ::fabs(value); }
> 
> Doesn't the existing STL math library (<cmath>, or maybe <cfloat>) provide template methods for these already?

Yes!  std::abs() provides overloaded arguments for floating point types, so I'll use that instead.

> Source/WTF/wtf/NumericConversion.h:168
> +inline float narrowPrecisionTo(const double& value) { return static_cast<float>(value); }
> 
> Is there any advantage to range checks like you did for double->int here? IIRC, floats have about an order of magnitude smaller range of values they can hold (aside from the obvious precision differences).

I suppose it depends on where you care about the loss of precision happening.  In larger numbers, the loss of precision occurs to the left of the decimal place.  In smaller numbers, it's to the right of the decimal place.  I'm not sure what a good rule of thumb here is, though, so I did the same thing that Source/WebCore/platform/FloatPrecision.h does now.

[Darin Adler]

Comment on attachment 228131 [details]
Patch v1

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

> Source/WTF/wtf/NumericConversion.h:36
> +T narrowPrecisionTo(const U& value);

I don’t think we need const U& here. Just U should be fine. I see no advantage to const int&, etc.

> Source/WTF/wtf/NumericConversion.h:39
> +T to(const U& value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:96
> +template<>
> +inline int narrowPrecisionTo(const float& value)
> +{
> +    ASSERT(!isinf(value));
> +    ASSERT(!isnan(value));
> +    ASSERT(static_cast<double>(value) >= static_cast<double>(std::numeric_limits<int>::min()));
> +    ASSERT(static_cast<double>(value) <= static_cast<double>(std::numeric_limits<int>::max()));
> +    return static_cast<int>(value);
> +}

I don’t like the name here. This doesn’t narrow precision, it truncates to an integer, which doesn’t seem like the same thing. Further, besides truncation, there are other operations one could do to convert a float to an int, such as rounding.

> Source/WTF/wtf/NumericConversion.h:99
> +inline int narrowPrecisionTo(const double& value)

Ditto.

> Source/WTF/wtf/NumericConversion.h:115
> +    ASSERT(static_cast<double>(result) == static_cast<double>(value));

How important is this assertion? It seems fine to convert an int to a float in cases where we lose a small of amount precision, in most cases. Asserting we lost no precision seems overzealous. The name to<int> doesn’t justify this strictness to me. If we really need such functions, I think they should have different names.

> Source/WTF/wtf/NumericConversion.h:123
> +    ASSERT(static_cast<long>(result) == value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:131
> +    ASSERT(static_cast<long long>(result) == value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:141
> +    ASSERT(static_cast<double>(result) == static_cast<double>(value));

Ditto.

> Source/WTF/wtf/NumericConversion.h:149
> +    ASSERT(static_cast<unsigned long>(result) == value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:157
> +    ASSERT(static_cast<unsigned long long>(result) == value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:180
> +    ASSERT(static_cast<long>(result) == value);

Same comment for double.

> Source/WTF/wtf/NumericConversion.h:188
> +    ASSERT(static_cast<long long>(result) == value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:201
> +    ASSERT(static_cast<unsigned long>(result) == value);

Ditto.

> Source/WTF/wtf/NumericConversion.h:209
> +    ASSERT(static_cast<unsigned long long>(result) == value);

Ditto.

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:126
> +                    float dragAngle = atanf(to<float>(abs(deltaFromStart.height())) / to<float>(abs(deltaFromStart.width())));

Does not seem like we want to do range checking here. We don’t care if the height or width is so large it doesn’t fit with full precision into a float. Approximation is fine.

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:144
> +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollLeft : ScrollRight, ScrollByPixel, to<float>(abs(delta)));

Does not seem like we want to do range checking here. Do we really need to check if the delta is so large that it won’t fit with full precision into a float?

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:150
> +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollUp : ScrollDown, ScrollByPixel, to<float>(abs(delta)));

Ditto.

> Source/WebCore/platform/ios/wak/WKView.mm:253
> +                         || (narrowPrecisionTo<int>(abs<CGFloat>(origMarginsTotal)) == 1)) {

I don’t understand why abs<CGFloat> is needed here. Wouldn’t std::abs do the right thing without specifying an explicit type?

Any why are we truncating to integer here. Is that really what we want rather than, say, rounding?

[Csaba Osztrogonác]

Comment on attachment 228131 [details]
Patch v1

Cleared Brent Fulgham's review+ from obsolete attachment 228131 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[David Kilzer (:ddkilzer)]

Comment on attachment 228131 [details]
Patch v1

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

>> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:126
>> +                    float dragAngle = atanf(to<float>(abs(deltaFromStart.height())) / to<float>(abs(deltaFromStart.width())));
> 
> Does not seem like we want to do range checking here. We don’t care if the height or width is so large it doesn’t fit with full precision into a float. Approximation is fine.

Range checks will be removed from to<float>() in the next patch.  After talking with Simon, we do want to perform floating point arithmetic here, so as a result I'm not going to change the above line in the next patch.

>> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:144
>> +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollLeft : ScrollRight, ScrollByPixel, to<float>(abs(delta)));
> 
> Does not seem like we want to do range checking here. Do we really need to check if the delta is so large that it won’t fit with full precision into a float?

Range checks will be removed from to<float>() in the next patch.  After talking with Simon, we do want to perform floating point arithmetic here, so as a result I'm not going to change the above line in the next patch.

>> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:150
>> +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollUp : ScrollDown, ScrollByPixel, to<float>(abs(delta)));
> 
> Ditto.

Ditto.

>> Source/WebCore/platform/ios/wak/WKView.mm:253
>> +                         || (narrowPrecisionTo<int>(abs<CGFloat>(origMarginsTotal)) == 1)) {
> 
> I don’t understand why abs<CGFloat> is needed here. Wouldn’t std::abs do the right thing without specifying an explicit type?
> 
> Any why are we truncating to integer here. Is that really what we want rather than, say, rounding?

I will switch to std::abs in the next patch.

In looking at the code with Simon, we suspect that back when this code was written (in August 2005), the height values would always be integral (even though they were stored as floating point values).  We also briefly searched to see if this code is even still used, but we couldn't definitely rule it out, so I'd prefer to keep the original behavior here for this patch.

[David Kilzer (:ddkilzer)]

Created attachment 238225 [details]
Patch v2

[WebKit Commit Bot]

Attachment 238225 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/wak/WKView.mm:252:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> Created an attachment (id=238225) [details]
> Patch v2

Changes since Patch v1:
- Replaced "const U&" with "U".
- Renamed narrowPrecisionTo<>() to truncateTo<>().
- Removed ASSERT()s where Darin thought they were unnecessary.
- Removed abs<CGFloat>() and used std::abs() instead in WKView.mm.  (Did not change code otherwise.  Risk/reward didn't see correct for this legacy code, and it's out of scope for this change.)

[Darin Adler]

Comment on attachment 238225 [details]
Patch v2

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

> Source/WTF/ChangeLog:10
> +        - to<T>(U) converts type U to type T with runtime bounds checks
> +          in Debug builds for integral types.

Since by runtime bounds checks you mean assertions, I am a little puzzled. We would only assert something that’s guaranteed. Since there are no uses of to<int> it’s hard for me to argue one way or another whether there is a guarantee, but this doesn’t sound great to me.

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:132
> +                    float dragAngle = atanf(to<float>(abs(deltaFromStart.height())) / to<float>(abs(deltaFromStart.width())));

I don’t see any benefit to using to<float> here instead of static_cast<float>. Is there a benefit I am missing?

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:150
> +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollLeft : ScrollRight, ScrollByPixel, to<float>(abs(delta)));

Ditto.

> Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:156
> +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollUp : ScrollDown, ScrollByPixel, to<float>(abs(delta)));

Ditto.

> Source/WebCore/platform/ios/wak/WKView.mm:252
>                  else if (origMarginsTotal == 0.0f 
> -                         || (abs(origMarginsTotal) == 1)) {
> +                         || (truncateTo<int>(std::abs(origMarginsTotal)) == 1)) {

I don’t think that truncating is what we want to do here. Lets instead write out the code in a way that makes it obvious what it actually does. Here's what the code does:

    if (origMarginsTotal == 0 || (origMarginsTotal > -2 && origMarginsTotal <= -1) || (origMarginsTotal >= 1 && origMarginsTotal < 2))

I think this rule is wrong, and rewriting it to use std::abs and truncateTo just makes it seem like it makes sense, but obscures what is actually happening. Writing the way the way I suggest above to make it clear what it really does might make someone consider fixing it. One step in the right direction would be to get rid of the strange special case for the range (0,1). Like this:

    if (origMarginsTotal > -2 && origMarginsTotal < 2)

That seems like it might be a helpful change, although it is a change in behavior.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (From update of attachment 238225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238225&action=review
> 
> > Source/WTF/ChangeLog:10
> > +        - to<T>(U) converts type U to type T with runtime bounds checks
> > +          in Debug builds for integral types.
> 
> Since by runtime bounds checks you mean assertions, I am a little puzzled. We would only assert something that’s guaranteed. Since there are no uses of to<int> it’s hard for me to argue one way or another whether there is a guarantee, but this doesn’t sound great to me.

I can't tell whether your objection is with this comment in the ChangeLog, or the actual to<int> methods.

The goal of the assertion checks for to<int> methods is to catch bugs in debug builds were integer truncation is happening unexpectedly.  I guess if that's a non-goal, then we don't need the to<int> methods.

> > Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:132
> > +                    float dragAngle = atanf(to<float>(abs(deltaFromStart.height())) / to<float>(abs(deltaFromStart.width())));
> 
> I don’t see any benefit to using to<float> here instead of static_cast<float>. Is there a benefit I am missing?

static_cast<float> is a rather large hammer that will change any base numeric type to a float.

The to<float> methods are more explicit and can be tailored to behave differently (in Debug builds) for different arguments.  (I guess most of the assertions I had in earlier patches were ripped out, so maybe this doesn't matter anymore.)

> > Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:150
> > +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollLeft : ScrollRight, ScrollByPixel, to<float>(abs(delta)));
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/ScrollAnimatorIOS.mm:156
> > +        handled |= m_scrollableAreaForTouchSequence->scroll(delta < 0 ? ScrollUp : ScrollDown, ScrollByPixel, to<float>(abs(delta)));
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/wak/WKView.mm:252
> >                  else if (origMarginsTotal == 0.0f 
> > -                         || (abs(origMarginsTotal) == 1)) {
> > +                         || (truncateTo<int>(std::abs(origMarginsTotal)) == 1)) {
> 
> I don’t think that truncating is what we want to do here. Lets instead write out the code in a way that makes it obvious what it actually does. Here's what the code does:
> 
>     if (origMarginsTotal == 0 || (origMarginsTotal > -2 && origMarginsTotal <= -1) || (origMarginsTotal >= 1 && origMarginsTotal < 2))
> 
> I think this rule is wrong, and rewriting it to use std::abs and truncateTo just makes it seem like it makes sense, but obscures what is actually happening. Writing the way the way I suggest above to make it clear what it really does might make someone consider fixing it. One step in the right direction would be to get rid of the strange special case for the range (0,1). Like this:
> 
>     if (origMarginsTotal > -2 && origMarginsTotal < 2)
> 
> That seems like it might be a helpful change, although it is a change in behavior.

Okay.

[Andy Estes]

In the interest of moving forward quickly, I'm going to post a patch that fixes these warnings with regular casts and let Dave continue with the NumericConversion.h work as a follow-up.

[Andy Estes]

Created attachment 239372 [details]
Patch

[Andy Estes]

Committed r174383: <http://trac.webkit.org/changeset/174383>