Bug 202090 – clang-tidy: Fix unnecessary copy/ref churn of for loop variables in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Running clang-tidy on WebCore resulted in these potential performance improvements to prevent object copies or reference churn in for loop variables:

Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:174:27: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
                for (auto request : fpsPssh.initDataBox().requests()) {
                          ^
                     const  &
--
Source/WebCore/svg/SVGStringList.h:78:19: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
        for (auto string : m_items) {
                  ^
             const  &
--
Source/WebCore/./platform/ios/PlatformPasteboardIOS.mm:729:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto type : types)
              ^
         const  &
--
Source/WebCore/./accessibility/AccessibilityObject.cpp:493:19: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
        for (auto misspelling : misspellings) {
                  ^
             const  &
--
Source/WebCore/./accessibility/AccessibilityObject.cpp:985:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto textRange : operation.textRanges) {
              ^
         const  &
--
Source/WebCore/./html/HTMLSlotElement.cpp:119:23: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (RefPtr<Node> node : *assignedNodes) {
                      ^
         const       &
--
Source/WebCore/./layout/inlineformatting/InlineFormattingContextLineLayout.cpp:368:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto floatItem : floats) {
              ^
         const  &
--
Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:664:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto loadResult : _loadResults) {
              ^
         const  &
--
Source/WebCore/html/track/WebVTTParser.cpp:392:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto rule : childRules) {
              ^
         const  &
--
Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:81:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto transceiver : m_transceivers)
              ^
         const  &

[Radar WebKit Bug Importer]

<rdar://problem/55600665>

[David Kilzer (:ddkilzer)]

Created attachment 379343 [details]
Patch v1

[Daniel Bates]

Comment on attachment 379343 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=379343&action=review
This patch looks good.

> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:80
> +    std::vector<rtc::scoped_refptr<webrtc::RtpTransceiverInterface>> transceivers(m_transceivers.size());

This change doubles the size of the final vector. This constructor takes a vector size (read: not capacity).

[David Kilzer (:ddkilzer)]

Comment on attachment 379343 [details]
Patch v1

This causes failures in fast/mediastream tests.  Removing r+/cq? flags.

[David Kilzer (:ddkilzer)]

Created attachment 379347 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Comment on attachment 379343 [details]
Patch v1

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

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:80
>> +    std::vector<rtc::scoped_refptr<webrtc::RtpTransceiverInterface>> transceivers(m_transceivers.size());
> 
> This change doubles the size of the final vector. This constructor takes a vector size (read: not capacity).

It also instantiates m_transceivers.size() objects using the default constructor, which is not what I wanted!

Switched to use std::vector<>::reserve() in Patch v2.

Thanks!

[Daniel Bates]

Comment on attachment 379347 [details]
Patch v2

Thanks for updating the patch. Looks good.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Created attachment 379347 [details]
> Patch v2

ios-wk2 ews failure:

media/presentationmodechanged-fired-once.html

This test is crashing intermittently without this patch:

<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media/presentationmodechanged-fired-once.html>

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 379347 [details]:

imported/w3c/web-platform-tests/websockets/bufferedAmount-unchanged-by-sync-xhr.any.worker.html bug 202003 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 379347 [details]
Patch v2

Clearing flags on attachment: 379347

Committed r250201: <https://trac.webkit.org/changeset/250201>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.