Bug 57337 – Fix compilation with Solaris 10/Sun Studio 12 CC : types must match on ?:

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Taylor]

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*).

[Ben Taylor]

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.

[WebKit Review Bot]

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.

[Ben Taylor]

ChangeLog entry has a bug number in it.  First line.  Is the style checker so pedantic, that it can't handle a trailing comma?

[Eric Seidel (no email)]

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?

[Ben Taylor]

(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.

[Ben Taylor]

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

[Ben Taylor]

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...

[Eric Seidel (no email)]

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.

[Ben Taylor]

(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?

[Ben Taylor]

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.

[Eric Seidel (no email)]

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

[Ben Taylor]

(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.

[Ben Taylor]

(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.

[Ben Taylor]

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.

[Eric Seidel (no email)]

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.)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 89050 [details]:

fast/loader/recursive-before-unload-crash.html bug 50880 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.