Bug 162302 – [GTK] webkit-gtk-2.14.0 build failure of ScrollbarThemeGtk with libc++

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jeremy Huddleston Sequoia]

2.14.0 fails to build with the following error using clang+libc++ on darwin:

webkitgtk-2.14.0/Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:547:35: error: call to 'abs' is ambiguous
                contentsRect.move(std::abs(rect.width() - preferredSize.width()), 0);
                                  ^~~~~~~~
../include/c++/v1/cmath:664:1: note: candidate function
abs(float __lcpp_x) _NOEXCEPT {return fabsf(__lcpp_x);}
^
../include/c++/v1/cmath:668:1: note: candidate function
abs(double __lcpp_x) _NOEXCEPT {return fabs(__lcpp_x);}
^
../include/c++/v1/cmath:672:1: note: candidate function
abs(long double __lcpp_x) _NOEXCEPT {return fabsl(__lcpp_x);}
^
webkitgtk-2.14.0/Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:552:34: error: call to 'abs' is ambiguous
            contentsRect.move(0, std::abs(rect.height() - preferredSize.height()));
                                 ^~~~~~~~
../include/c++/v1/cmath:664:1: note: candidate function
abs(float __lcpp_x) _NOEXCEPT {return fabsf(__lcpp_x);}
^
../include/c++/v1/cmath:668:1: note: candidate function
abs(double __lcpp_x) _NOEXCEPT {return fabs(__lcpp_x);}
^
../include/c++/v1/cmath:672:1: note: candidate function
abs(long double __lcpp_x) _NOEXCEPT {return fabsl(__lcpp_x);}
^
2 errors generated.

Casting the call site to (double) addresses the issue.

[Jeremy Huddleston Sequoia]

Created attachment 289382 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


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

[Jeremy Huddleston Sequoia]

Created attachment 289387 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

[Alex Christensen]

Comment on attachment 289387 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

If such a cast is needed, please use static_cast instead of a c-style cast.

[Michael Catanzaro]

Comment on attachment 289387 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

Yes, as Alex said.

[Carlos Garcia Campos]

Comment on attachment 289387 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

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

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:547
> -                contentsRect.move(std::abs(rect.width() - preferredSize.width()), 0);
> +                contentsRect.move(std::abs((double)(rect.width() - preferredSize.width())), 0);

I think it would be cleaner to cast the result of abs, because both rect and preferredSize are ints, instead of using the abs overload for floats with integers, we could use the int one and cast the result. Another way to disambiguate this could be using std::fabs, adn then we don't need any ugly cast.

[Jeremy Huddleston Sequoia]

Oh, I wonder if it's not picking up the integral std::abs() because of a missing #include <cstdlib>.  Giving that a try.

[Jeremy Huddleston Sequoia]

Created attachment 289435 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

Yep, changed patch to include <cstdlib>, so the integer versions are available.

[WebKit Commit Bot]

Comment on attachment 289435 [details]
0002-GTK-Fix-build-failure-of-ScrollbarThemeGtk-with-libc

Clearing flags on attachment: 289435

Committed r206205: <http://trac.webkit.org/changeset/206205>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(In reply to comment #7)
> Oh, I wonder if it's not picking up the integral std::abs() because of a
> missing #include <cstdlib>.  Giving that a try.

Ugh, let's remember to check for this whenever we see a patch that changes std::abs. It's not the first time this has been an issue, but I forgot.