REOPENED 174631
[Xcode] Enable warnings recommended by Xcode 9
https://bugs.webkit.org/show_bug.cgi?id=174631
Summary [Xcode] Enable warnings recommended by Xcode 9
Andy Estes
Reported 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
Attachments
Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING (15.98 KB, patch)
2017-07-18 09:20 PDT, Andy Estes
no flags
Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION (17.47 KB, patch)
2017-07-18 11:10 PDT, Andy Estes
no flags
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION (19.30 KB, patch)
2017-07-18 14:20 PDT, Andy Estes
no flags
Enable CLANG_WARN_OBJC_LITERAL_CONVERSION (19.58 KB, patch)
2017-07-18 14:32 PDT, Andy Estes
no flags
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS (29.02 KB, patch)
2017-07-18 16:35 PDT, Andy Estes
thorton: review+
commit-queue: commit-queue-
Enable CLANG_WARN_RANGE_LOOP_ANALYSIS (29.06 KB, patch)
2017-07-18 21:46 PDT, Andy Estes
no flags
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
Andy Estes
Comment 1 2017-07-18 09:20:02 PDT
Created attachment 315799 [details] Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING
Build Bot
Comment 2 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
Andy Estes
Comment 3 2017-07-18 11:10:52 PDT
Created attachment 315807 [details] Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION
WebKit Commit Bot
Comment 4 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>
Andy Estes
Comment 5 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.
mitz
Comment 6 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.
Andy Estes
Comment 7 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.
Andy Estes
Comment 8 2017-07-18 14:20:04 PDT
Created attachment 315836 [details] Enable CLANG_WARN_OBJC_LITERAL_CONVERSION
WebKit Commit Bot
Comment 9 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>
Andy Estes
Comment 10 2017-07-18 14:32:36 PDT
Created attachment 315841 [details] Enable CLANG_WARN_OBJC_LITERAL_CONVERSION
mitz
Comment 11 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*.
Andy Estes
Comment 12 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:.
Andy Estes
Comment 13 2017-07-18 16:35:26 PDT
Created attachment 315856 [details] Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 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
Andy Estes
Comment 16 2017-07-18 21:46:27 PDT
Created attachment 315888 [details] Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2017-07-18 22:47:30 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 19 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.
Andy Estes
Comment 20 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.
Andy Estes
Comment 21 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.
Andy Estes
Comment 22 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.
Andy Estes
Comment 23 2017-07-19 10:08:45 PDT
Reopening to attach new patch.
Andy Estes
Comment 24 2017-07-19 10:08:46 PDT
Created attachment 315929 [details] Use a cast to work around clang's false -Wobjc-literal-conversion warnings
Darin Adler
Comment 25 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.
WebKit Commit Bot
Comment 26 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>
WebKit Commit Bot
Comment 27 2017-07-19 11:16:40 PDT
All reviewed patches have been landed. Closing bug.
Andy Estes
Comment 28 2017-07-19 11:25:11 PDT
Still need to enable CLANG_WARN_STRICT_PROTOTYPES and CLANG_WARN_COMMA.
Tim Horton
Comment 29 2017-07-19 11:32:49 PDT
... good luck with strict prototypes :)
Note You need to log in before you can comment on or make changes to this bug.