WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug