Bug 174631 - [Xcode] Enable warnings recommended by Xcode 9
Summary: [Xcode] Enable warnings recommended by Xcode 9
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-18 08:47 PDT by Andy Estes
Modified: 2017-07-19 11:32 PDT (History)
19 users (show)

See Also:


Attachments
Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING (15.98 KB, patch)
2017-07-18 09:20 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION (17.47 KB, patch)
2017-07-18 11:10 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION (19.30 KB, patch)
2017-07-18 14:20 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION (19.58 KB, patch)
2017-07-18 14:32 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS (29.02 KB, patch)
2017-07-18 16:35 PDT, Andy Estes
thorton: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS (29.06 KB, patch)
2017-07-18 21:46 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Use a cast to work around clang's false -Wobjc-literal-conversion warnings (3.55 KB, patch)
2017-07-19 10:08 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2017-07-18 08:47:12 PDT
Xcode recommends we enable some new warnings:

CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING
CLANG_WARN_NON_LITERAL_NULL_CONVERSION
CLANG_WARN_OBJC_LITERAL_CONVERSION
CLANG_WARN_RANGE_LOOP_ANALYSIS
CLANG_WARN_STRICT_PROTOTYPES
CLANG_WARN_COMMA
Comment 1 Andy Estes 2017-07-18 09:20:02 PDT
Created attachment 315799 [details]
Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING
Comment 2 Build Bot 2017-07-18 09:21:00 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Andy Estes 2017-07-18 11:10:52 PDT
Created attachment 315807 [details]
Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION
Comment 4 WebKit Commit Bot 2017-07-18 11:39:15 PDT
Comment on attachment 315799 [details]
Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING

Clearing flags on attachment: 315799

Committed r219618: <http://trac.webkit.org/changeset/219618>
Comment 5 Andy Estes 2017-07-18 13:21:41 PDT
We can't enable CLANG_WARN_OBJC_LITERAL_CONVERSION in WebCore because of false positives from clang. I filed rdar://problem/33383354 about this.
Comment 6 mitz 2017-07-18 13:31:15 PDT
(In reply to Andy Estes from comment #5)
> We can't enable CLANG_WARN_OBJC_LITERAL_CONVERSION in WebCore because of
> false positives from clang. I filed rdar://problem/33383354 about this.

Too many false positive to consider suppressing the warning just around them?

In the example you cited in <rdar://problem/33383354> it seems like the problem can be addressed by making the appropriate types and declarations visible to the compiler.
Comment 7 Andy Estes 2017-07-18 13:41:48 PDT
(In reply to mitz from comment #6)
> (In reply to Andy Estes from comment #5)
> > We can't enable CLANG_WARN_OBJC_LITERAL_CONVERSION in WebCore because of
> > false positives from clang. I filed rdar://problem/33383354 about this.
> 
> Too many false positive to consider suppressing the warning just around them?

No, not too many. I didn't think of suppressing the warning, but that definitely a better idea!
> 
> In the example you cited in <rdar://problem/33383354> it seems like the
> problem can be addressed by making the appropriate types and declarations
> visible to the compiler.

The problems I'm seeing are the calls to [m_pasteboard setItems:...] in PlatformPasteboardIOS.mm (that file includes UIPasteboard.h).

I think m_pasteboard needs to remain id-typed since at runtime it is either a UIPasteboard or a WebItemProviderPasteboard.
Comment 8 Andy Estes 2017-07-18 14:20:04 PDT
Created attachment 315836 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION
Comment 9 WebKit Commit Bot 2017-07-18 14:23:56 PDT
Comment on attachment 315807 [details]
Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION

Clearing flags on attachment: 315807

Committed r219628: <http://trac.webkit.org/changeset/219628>
Comment 10 Andy Estes 2017-07-18 14:32:36 PDT
Created attachment 315841 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION
Comment 11 mitz 2017-07-18 14:55:06 PDT
Comment on attachment 315841 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:282
>      [m_pasteboard setItems:@[representations.get()]];

Alternatively we could just cast to (UIPasteboard *), so at least the compiler is checking *something*.
Comment 12 Andy Estes 2017-07-18 15:03:50 PDT
(In reply to mitz from comment #11)
> Comment on attachment 315841 [details]
> Enable CLANG_WARN_OBJC_LITERAL_CONVERSION
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315841&action=review
> 
> > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:282
> >      [m_pasteboard setItems:@[representations.get()]];
> 
> Alternatively we could just cast to (UIPasteboard *), so at least the
> compiler is checking *something*.

I guess this would be ok since when m_pasteboard is a WebItemProviderPasteboard it will respond to -setItemsUsingRegistrationInfoLists:.
Comment 13 Andy Estes 2017-07-18 16:35:26 PDT
Created attachment 315856 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
Comment 14 WebKit Commit Bot 2017-07-18 18:05:49 PDT
Comment on attachment 315841 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION

Clearing flags on attachment: 315841

Committed r219644: <http://trac.webkit.org/changeset/219644>
Comment 15 WebKit Commit Bot 2017-07-18 21:34:06 PDT
Comment on attachment 315856 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS

Rejecting attachment 315856 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 315856, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
Configurations/Base.xcconfig
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file Tools/TestWebKitAPI/Configurations/Base.xcconfig.rej
patching file Tools/WebKitTestRunner/Configurations/Base.xcconfig
Hunk #1 FAILED at 34.
1 out of 1 hunk FAILED -- saving rejects to file Tools/WebKitTestRunner/Configurations/Base.xcconfig.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Tim Horton']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4145805
Comment 16 Andy Estes 2017-07-18 21:46:27 PDT
Created attachment 315888 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
Comment 17 WebKit Commit Bot 2017-07-18 22:47:28 PDT
Comment on attachment 315888 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS

Clearing flags on attachment: 315888

Committed r219648: <http://trac.webkit.org/changeset/219648>
Comment 18 WebKit Commit Bot 2017-07-18 22:47:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Darin Adler 2017-07-18 22:57:01 PDT
Comment on attachment 315888 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS

I don’t understand these changes at all. Not why clang is warning about these, nor why it’s OK to use auto instead of auto& or const auto& in these various cases. Seems like it will lead to unnecessary copying rather than operating on items in place.
Comment 20 Andy Estes 2017-07-19 09:01:19 PDT
(In reply to Darin Adler from comment #19)
> Comment on attachment 315888 [details]
> Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
> 
> I don’t understand these changes at all. Not why clang is warning about
> these, nor why it’s OK to use auto instead of auto& or const auto& in these
> various cases. Seems like it will lead to unnecessary copying rather than
> operating on items in place.

In the cases where a value type is used instead of a reference type, that's just reflecting the fact that copies were already being made during iteration. Previously we had references that were bound to temporaries, and now we don't, but there is no new copying taking place.
Comment 21 Andy Estes 2017-07-19 09:43:53 PDT
(In reply to Darin Adler from comment #19)
> Comment on attachment 315888 [details]
> Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
> 
> I don’t understand these changes at all. Not why clang is warning about
> these, nor why it’s OK to use auto instead of auto& or const auto& in these
> various cases. Seems like it will lead to unnecessary copying rather than
> operating on items in place.

Here's what one of the warnings looked like:

/Users/andy/Code/OpenSource/Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:249:22: error: loop variable 'mimeType' is always a copy because the range of type 'ElementsOfTypeRange<API::String>' (aka 'WTF::IteratorRange<WTF::TransformIterator<API::Array::GetObjectTransform<API::String>, WTF::FilterIterator<API::Array::IsTypePredicate<API::String>, const WTF::RefPtr<API::Object> *> > >') does not return a reference [-Werror,-Wrange-loop-analysis]
    for (const auto& mimeType : acceptMimeTypes->elementsOfType<API::String>())
                     ^
/Users/andy/Code/OpenSource/Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:249:10: note: use non-reference type 'const API::String *'
    for (const auto& mimeType : acceptMimeTypes->elementsOfType<API::String>())
         ^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Comment 22 Andy Estes 2017-07-19 09:46:56 PDT
(In reply to Darin Adler from comment #19)
> Comment on attachment 315888 [details]
> Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
> 
> I don’t understand these changes at all. Not why clang is warning about
> these, nor why it’s OK to use auto instead of auto& or const auto& in these
> various cases. Seems like it will lead to unnecessary copying rather than
> operating on items in place.

It's also worth pointing out that we're not copying anything expensive; they are either pointers or integers in the cases where I made changes.
Comment 23 Andy Estes 2017-07-19 10:08:45 PDT
Reopening to attach new patch.
Comment 24 Andy Estes 2017-07-19 10:08:46 PDT
Created attachment 315929 [details]
Use a cast to work around clang's false -Wobjc-literal-conversion warnings
Comment 25 Darin Adler 2017-07-19 10:16:48 PDT
(In reply to Andy Estes from comment #22)
> It's also worth pointing out that we're not copying anything expensive; they
> are either pointers or integers in the cases where I made changes.

OK. I misread the one I studied, involving GridSpan. I thought it was copying a GridSpan, but it’s copying an unsigned.
Comment 26 WebKit Commit Bot 2017-07-19 11:16:38 PDT
Comment on attachment 315929 [details]
Use a cast to work around clang's false -Wobjc-literal-conversion warnings

Clearing flags on attachment: 315929

Committed r219660: <http://trac.webkit.org/changeset/219660>
Comment 27 WebKit Commit Bot 2017-07-19 11:16:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Andy Estes 2017-07-19 11:25:11 PDT
Still need to enable CLANG_WARN_STRICT_PROTOTYPES and CLANG_WARN_COMMA.
Comment 29 Tim Horton 2017-07-19 11:32:49 PDT
... good luck with strict prototypes :)