Bug 57337 - Fix compilation with Solaris 10/Sun Studio 12 CC : types must match on ?:
Summary: Fix compilation with Solaris 10/Sun Studio 12 CC : types must match on ?:
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-29 07:17 PDT by Ben Taylor
Modified: 2011-04-13 07:59 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler (1.25 KB, patch)
2011-03-29 07:31 PDT, Ben Taylor
eric: review-
Details | Formatted Diff | Diff
Updated patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler, changed to static_cast (1.12 KB, patch)
2011-03-29 18:54 PDT, Ben Taylor
no flags Details | Formatted Diff | Diff
Updated patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler, changed to static_cast and fix style issue (extra space) (1.12 KB, patch)
2011-03-29 19:02 PDT, Ben Taylor
no flags Details | Formatted Diff | Diff
Fixed patch that correct the compile, without trying to static_cast a variable that is already the right type (obviously compiler issue) (1.11 KB, patch)
2011-04-03 06:49 PDT, Ben Taylor
no flags Details | Formatted Diff | Diff
Updated patch which explains that we are working around Solaris Studio 12/12.1/12.2 compiler bug with ternary operations. (1.28 KB, patch)
2011-04-11 12:24 PDT, Ben Taylor
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Taylor 2011-03-29 07:17:34 PDT
in Source/WebCore/dom/NodeRenderStyle.h, the construct in line 36

return m_renderer ? m_renderer->style() : nonRendererRenderStyle();

fails to compile on Solaris 10/Sun Studio 12 CC, with the error:

Error: Different types for "?:" (bool and WebCore::RenderStyle*).
Comment 1 Ben Taylor 2011-03-29 07:31:53 PDT
Created attachment 87320 [details]
Proposed patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler

Fix for the above error, just cast the first conditional return to the appropriate type.
Comment 2 WebKit Review Bot 2011-03-29 07:34:49 PDT
Attachment 87320 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ben Taylor 2011-03-29 08:15:09 PDT
ChangeLog entry has a bug number in it.  First line.  Is the style checker so pedantic, that it can't handle a trailing comma?
Comment 4 Eric Seidel (no email) 2011-03-29 13:04:51 PDT
Comment on attachment 87320 [details]
Proposed patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler

We don't use c-style casts in WebKit.  static_cast is your friend. :)  How is style() not a RenderStyle?
Comment 5 Ben Taylor 2011-03-29 13:22:15 PDT
(In reply to comment #4)
> (From update of attachment 87320 [details])
> We don't use c-style casts in WebKit.  static_cast is your friend. :)  How is style() not a RenderStyle?

The compiler reports this as a mismatch.  I can recode as a static_cast, or do as Thiago did in his original patch and say:

+    if (m_renderer)
+        return m_renderer->style();
+    return nonRendererRenderStyle();

I follow your guidance.
Comment 6 Ben Taylor 2011-03-29 18:54:37 PDT
Created attachment 87449 [details]
Updated patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler, changed to static_cast

updated patch using recommended static_cast
Comment 7 Ben Taylor 2011-03-29 19:02:50 PDT
Created attachment 87450 [details]
Updated patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler, changed to static_cast and fix style issue (extra space)

fix extra space in the patch issue, that would have surely flagged the style bot...
Comment 8 Eric Seidel (no email) 2011-03-31 09:53:15 PDT
Comment on attachment 87450 [details]
Updated patch which compiles correctly in qt-4.7.2 with webkit enabled on Solaris 10 with SS12 C++ compiler, changed to static_cast and fix style issue (extra space)

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderObject.h#L598

Note that style() is already a RenderStyle*.  Something is fishy here.
Comment 9 Ben Taylor 2011-03-31 10:23:52 PDT
(In reply to comment #8)
> (From update of attachment 87450 [details])
> http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderObject.h#L598
> 
> Note that style() is already a RenderStyle*.  Something is fishy here.

Would you prefer I used Thiago's method?
Comment 10 Ben Taylor 2011-04-03 06:49:00 PDT
Created attachment 88008 [details]
Fixed patch that correct the compile, without trying to static_cast a variable that is already the right type (obviously compiler issue)

This was Thiago Macieria's version of this patch.
Comment 11 Eric Seidel (no email) 2011-04-08 08:24:14 PDT
Comment on attachment 88008 [details]
Fixed patch that correct the compile, without trying to static_cast a variable that is already the right type (obviously compiler issue)

This looks fine, but I would prefer if you would add a comment here:
// Using a ternary here confuses The Solaris 10 compiler:
// http://link-to-solaris-10-compiler-bug

That way someone is unlikely to break it again.

Do we understand why only this one ternary fails?  We have lots of similar ternaries througout the project
Comment 12 Ben Taylor 2011-04-08 08:32:38 PDT
(In reply to comment #11)
> (From update of attachment 88008 [details])
> This looks fine, but I would prefer if you would add a comment here:
> // Using a ternary here confuses The Solaris 10 compiler:
> // http://link-to-solaris-10-compiler-bug
> 
> That way someone is unlikely to break it again.
> 
> Do we understand why only this one ternary fails?  We have lots of similar ternaries througout the project

This is a weird one.  I've patched on bunch of the ternary operations (similar type things where the types were in fact different) over the last month for webkit.  I will research the bug and see what I can find, and hold off on a updated patch until I can identify it.
Comment 13 Ben Taylor 2011-04-11 12:11:43 PDT
(In reply to comment #11)
> (From update of attachment 88008 [details])
> This looks fine, but I would prefer if you would add a comment here:
> // Using a ternary here confuses The Solaris 10 compiler:
> // http://link-to-solaris-10-compiler-bug
> 
> That way someone is unlikely to break it again.
> 
> Do we understand why only this one ternary fails?  We have lots of similar ternaries througout the project

Bug id found.  If I remove the inline from the template, the problem goes away which confirms bug CR 6569194, "Problem with question operator binding in inline function".  However, the recommendation was to use the if clause to fix this problem as removing the inline was not a good work around.

Fixing up the comments and will resubmit the patch.  However, the Bug report is not publically available, so will just provide the bug number and description.
Comment 14 Ben Taylor 2011-04-11 12:24:52 PDT
Created attachment 89050 [details]
Updated patch which explains that we are working around Solaris Studio 12/12.1/12.2 compiler bug with ternary operations.

Patch updated with proper comments on why we fix the bug the way we did.
Comment 15 Eric Seidel (no email) 2011-04-13 06:53:57 PDT
Comment on attachment 89050 [details]
Updated patch which explains that we are working around Solaris Studio 12/12.1/12.2 compiler bug with ternary operations.

Thanks! This is a much much much better fix.  (Since it won't regress since it's commented, and with the bug number we now have some way of eventually removing it.)
Comment 16 WebKit Commit Bot 2011-04-13 07:55:50 PDT
The commit-queue encountered the following flaky tests while processing attachment 89050 [details]:

fast/loader/recursive-before-unload-crash.html bug 50880 (authors: beidson@apple.com and eric@webkit.org)
The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2011-04-13 07:59:12 PDT
Comment on attachment 89050 [details]
Updated patch which explains that we are working around Solaris Studio 12/12.1/12.2 compiler bug with ternary operations.

Clearing flags on attachment: 89050

Committed r83736: <http://trac.webkit.org/changeset/83736>
Comment 18 WebKit Commit Bot 2011-04-13 07:59:16 PDT
All reviewed patches have been landed.  Closing bug.