Bug 90692 - WebCore doesn't build with recent clang (w/ patch)
Summary: WebCore doesn't build with recent clang (w/ patch)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-06 09:44 PDT by Nuno Lopes
Modified: 2013-05-02 12:09 PDT (History)
13 users (show)

See Also:


Attachments
fix warnings by the new -Wunused-private-field (7.07 KB, patch)
2012-07-06 09:47 PDT, Nuno Lopes
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
fix warnings by the new -Wunused-private-field (take #2) (4.97 KB, patch)
2012-07-06 10:47 PDT, Nuno Lopes
no flags Details | Formatted Diff | Diff
fix warnings by the new -Wunused-private-field (take #3) (4.98 KB, patch)
2012-07-06 10:52 PDT, Nuno Lopes
rniwa: review-
rniwa: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nuno Lopes 2012-07-06 09:44:48 PDT
WebCore doesn't build with recent clang.
Comment 1 Nuno Lopes 2012-07-06 09:47:33 PDT
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.
Comment 2 WebKit Review Bot 2012-07-06 10:09:44 PDT
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
Comment 3 Early Warning System Bot 2012-07-06 10:26:44 PDT
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
Comment 4 Early Warning System Bot 2012-07-06 10:29:38 PDT
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
Comment 5 Nuno Lopes 2012-07-06 10:47:02 PDT
Created attachment 151101 [details]
fix warnings by the new -Wunused-private-field (take #2)
Comment 6 WebKit Review Bot 2012-07-06 10:49:56 PDT
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.
Comment 7 Nuno Lopes 2012-07-06 10:52:44 PDT
Created attachment 151103 [details]
fix warnings by the new -Wunused-private-field (take #3)
Comment 8 Alexey Proskuryakov 2012-07-06 12:08:45 PDT
Did you want to mark this for review, not just cq?
Comment 9 Nuno Lopes 2012-07-06 13:12:08 PDT
(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..
Comment 10 Alexey Proskuryakov 2012-07-06 23:48:18 PDT
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 cmarrin@apple.com // FIXME: This class is currently empty on Mac, but will get populated as 
 94083 cmarrin@apple.com // the restructuring in https://bugs.webkit.org/show_bug.cgi?id=66903 is done
Comment 11 Nuno Lopes 2012-07-09 09:55:20 PDT
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 cmarrin@apple.com // FIXME: This class is currently empty on Mac, but will get populated as 
>  94083 cmarrin@apple.com // 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!
Comment 12 Alexey Proskuryakov 2012-07-09 12:32:35 PDT
> 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.
Comment 13 Ryosuke Niwa 2012-07-19 16:29:14 PDT
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.
Comment 14 Alexey Proskuryakov 2013-02-12 20:36:03 PST
Can this bug be closed? WebKit successfully builds with clang that's presumably newer than what we had in July 2012.
Comment 15 Anders Carlsson 2013-05-02 12:09:19 PDT
Looks like this has been fixed.