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
Created attachment 315799 [details] Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Created attachment 315807 [details] Enable CLANG_WARN_NON_LITERAL_NULL_CONVERSION
Comment on attachment 315799 [details] Enable CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING Clearing flags on attachment: 315799 Committed r219618: <http://trac.webkit.org/changeset/219618>
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.
(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.
Created attachment 315836 [details] Enable CLANG_WARN_OBJC_LITERAL_CONVERSION
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>
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*.
(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:.
Created attachment 315856 [details] Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
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 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
Created attachment 315888 [details] Enable CLANG_WARN_RANGE_LOOP_ANALYSIS
Comment on attachment 315888 [details] Enable CLANG_WARN_RANGE_LOOP_ANALYSIS Clearing flags on attachment: 315888 Committed r219648: <http://trac.webkit.org/changeset/219648>
All reviewed patches have been landed. Closing bug.
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 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.
(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.
(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.
Reopening to attach new patch.
Created attachment 315929 [details] Use a cast to work around clang's false -Wobjc-literal-conversion warnings
(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 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>
Still need to enable CLANG_WARN_STRICT_PROTOTYPES and CLANG_WARN_COMMA.
... good luck with strict prototypes :)