Bug 90692 – WebCore doesn't build with recent clang (w/ patch)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nuno Lopes]

WebCore doesn't build with recent clang.

[Nuno Lopes]

Created attachment 151096 [details]
fix warnings by the new -Wunused-private-field

I commented out the field in GraphicsContext3DPrivate instead of removing it since it has a comment saying that eventually it will be used in the future.

[WebKit Review Bot]

Comment on attachment 151096 [details]
fix warnings by the new -Wunused-private-field

Attachment 151096 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13156171

[Early Warning System Bot]

Comment on attachment 151096 [details]
fix warnings by the new -Wunused-private-field

Attachment 151096 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13142531

[Early Warning System Bot]

Comment on attachment 151096 [details]
fix warnings by the new -Wunused-private-field

Attachment 151096 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13140545

[Nuno Lopes]

Created attachment 151101 [details]
fix warnings by the new -Wunused-private-field (take #2)

[WebKit Review Bot]

Attachment 151101 [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/platform/graphics/cg/ImageSourceCG.cpp:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 3 files


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

[Nuno Lopes]

Created attachment 151103 [details]
fix warnings by the new -Wunused-private-field (take #3)

[Alexey Proskuryakov]

Did you want to mark this for review, not just cq?

[Nuno Lopes]

(In reply to comment #8)
> Did you want to mark this for review, not just cq?

yes, thanks!  I still don't master all these things of webkit..

[Alexey Proskuryakov]

Comment on attachment 151103 [details]
fix warnings by the new -Wunused-private-field (take #3)

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

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:80
> +    (void)m_alphaOption;
> +    (void)m_gammaAndColorProfileOption;

Why did you choose to keep these? The (void) trick is not something we normally do in WebKit code.

If this is really necessary, please add a comment.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:61
> +        /* : m_graphicsContext3D(graphicsContext3D) */

As a strong rule, we don't check in commented out code. Not sure who can comment about the refactoring plans for this class.

 94083 [e-mail] // FIXME: This class is currently empty on Mac, but will get populated as 
 94083 [e-mail] // the restructuring in https://bugs.webkit.org/show_bug.cgi?id=66903 is done

[Nuno Lopes]

Comment on attachment 151103 [details]
fix warnings by the new -Wunused-private-field (take #3)

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

>> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:80
>> +    (void)m_gammaAndColorProfileOption;
> 
> Why did you choose to keep these? The (void) trick is not something we normally do in WebKit code.
> 
> If this is really necessary, please add a comment.

AFAICT, there are several implementations of ImageSource. Other implementation do use these parameters, so they cannot be removed.
Moreover, there's a comment just one line above (that cannot be seen here in the diff) that says:
" // FIXME: m_premultiplyAlpha is ignored in cg at the moment. "

So I assume someone will want to use the alpha stuff in the future.

>> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:61
>> +        /* : m_graphicsContext3D(graphicsContext3D) */
> 
> As a strong rule, we don't check in commented out code. Not sure who can comment about the refactoring plans for this class.
> 
>  94083 [e-mail] // FIXME: This class is currently empty on Mac, but will get populated as 
>  94083 [e-mail] // the restructuring in https://bugs.webkit.org/show_bug.cgi?id=66903 is done

I've no clue about this; I was just obeying the comment!

[Alexey Proskuryakov]

> AFAICT, there are several implementations of ImageSource. Other implementation do use these parameters, so they cannot be removed.

Normally, we'd ifdef the member variables out when they are not needed.

[Ryosuke Niwa]

Comment on attachment 151103 [details]
fix warnings by the new -Wunused-private-field (take #3)

r- ap's comment. Please upload a new patch addressing Alexey's comments.

[Alexey Proskuryakov]

Can this bug be closed? WebKit successfully builds with clang that's presumably newer than what we had in July 2012.

[Anders Carlsson]

Looks like this has been fixed.