Bug 33952

Summary: build failure on RVCT
Product: WebKit Reporter: Joe Mason <joenotcharles>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, darin, hausmann, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Description Flags
build fix
darin: review+
updated patch
style fix none

Description Joe Mason 2010-01-21 07:46:05 PST
RVCT 4.0 fails to compile in FEComponentTransfer.cpp - "ambiguous overload in function pow" (provided args are (int,float), possible overloads include (int,int), (float,float) and (double,double).  This patch casts the args to double to match the return value.
Comment 1 Joe Mason 2010-01-21 08:09:52 PST
Created attachment 47118 [details]
build fix
Comment 2 Darin Adler 2010-01-21 11:40:29 PST
Comment on attachment 47118 [details]
build fix

I'm unclear on what's ambiguous here and why RVCT is complaining. Where's the ambiguity? In C++, the expression i / 255.0 is unambiguously a double, so I can't see how adding a cast to that expression is helpful.

Further, pow(double, int) also does not seem ambiguous.

> -        double val = 255.0 * (transferFunction.amplitude * pow((i / 255.0), transferFunction.exponent) + transferFunction.offset);
> +        double val = 255.0 * (transferFunction.amplitude * pow((static_cast<double>(i) / 255.0), static_cast<double>(transferFunction.exponent)) + transferFunction.offset);

The change is OK. It's better to keep casts to a minimum if possible, so an approach that used local variables instead might be cleaner.

I'm going to say review+ because there's not a lot of downside here, but I think this is probably not a great change given a lack of understanding of what's ambiguous combined with the relatively ugly solution.
Comment 3 Darin Adler 2010-01-21 11:41:40 PST
I really don't see anything passing int, float here. Maybe double, float. Seems like yet another RVCT compiler bug. Is there any better way to support this flawed compiler?
Comment 4 Joe Mason 2010-01-21 11:46:03 PST
You're right, it's double,float, so only the second cast is needed.  I'll update the patch.
Comment 5 Joe Mason 2010-01-21 11:55:17 PST
Created attachment 47134 [details]
updated patch
Comment 6 WebKit Review Bot 2010-01-21 12:02:38 PST
Attachment 47134 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/filters/FEComponentTransfer.cpp:144:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Joe Mason 2010-01-21 12:12:20 PST
Created attachment 47136 [details]
style fix

Grr.  Knew I forgot something.
Comment 8 Darin Adler 2010-01-21 14:11:59 PST
Comment on attachment 47136 [details]
style fix

> +    Copyright (C) Research In Motion Limited 2010. All rights reserved.

I think there is a guideline about how big a chance is sufficient to assert copyright. This is probably small enough that it's not sufficient.

Change looks great. r=me
Comment 9 WebKit Commit Bot 2010-01-21 21:23:18 PST
Comment on attachment 47136 [details]
style fix

Clearing flags on attachment: 47136

Committed r53674: <http://trac.webkit.org/changeset/53674>
Comment 10 WebKit Commit Bot 2010-01-21 21:23:23 PST
All reviewed patches have been landed.  Closing bug.