Bug 174631 – [Xcode] Enable warnings recommended by Xcode 9

• Component: New Bugs
• Status: REOPENED

[Andy Estes]

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

[Andy Estes]

Created attachment 315799 [details]
Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING

[Build Bot]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Andy Estes]

Created attachment 315807 [details]
Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION

[WebKit Commit Bot]

Comment on attachment 315799 [details]
Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING

Clearing flags on attachment: 315799

Committed r219618: <http://trac.webkit.org/changeset/219618>

[Andy Estes]

We can't enable CLANG_WARN_OBJC_LITERAL_CONVERSION in WebCore because of false positives from clang. I filed rdar://problem/33383354 about this.

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

[Andy Estes]

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

[Andy Estes]

Created attachment 315836 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION

[WebKit Commit Bot]

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>

[Andy Estes]

Created attachment 315841 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION

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

[Andy Estes]

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

[Andy Estes]

Created attachment 315856 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS

[WebKit Commit Bot]

Comment on attachment 315841 [details]
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION

Clearing flags on attachment: 315841

Committed r219644: <http://trac.webkit.org/changeset/219644>

[WebKit Commit Bot]

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

[Andy Estes]

Created attachment 315888 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS

[WebKit Commit Bot]

Comment on attachment 315888 [details]
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS

Clearing flags on attachment: 315888

Committed r219648: <http://trac.webkit.org/changeset/219648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Andy Estes]

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

[Andy Estes]

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

[Andy Estes]

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

[Andy Estes]

Reopening to attach new patch.

[Andy Estes]

Created attachment 315929 [details]
Use a cast to work around clang's false -Wobjc-literal-conversion warnings

[Darin Adler]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

Still need to enable CLANG_WARN_STRICT_PROTOTYPES and CLANG_WARN_COMMA.

[Tim Horton]

... good luck with strict prototypes :)