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

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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

Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:514:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto domainName : domains) {
              ^
         const  &
--
Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:215:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto path : WebCore::PathUtilities::pathsWithShrinkWrappedRects(indicatedRects, 0)) {
              ^
         const  &
--
Source/WebKit/UIProcess/ios/WebDataListSuggestionsDropdownIOS.mm:172:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto suggestion : _suggestions) {
              ^
         const  &
--
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2491:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto action : actionsToPerform)
              ^
         const  &

[David Kilzer (:ddkilzer)]

Created attachment 379354 [details]
Patch v1

[Radar WebKit Bug Importer]

<rdar://problem/55609903>

[David Kilzer (:ddkilzer)]

iOS WK2 EWS test failure (timeout):

fast/scrolling/ios/click-events-after-long-press-during-momentum-scroll-in-main-frame.html

This test failure seems like a recent regression (happening in trunk), not related to this patch:

<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fscrolling%2Fios%2Fclick-events-after-long-press-during-momentum-scroll-in-main-frame.html>

[Darin Adler]

Comment on attachment 379354 [details]
Patch v1

In all these cases i would just use auto& rather than const auto&.

[WebKit Commit Bot]

Comment on attachment 379354 [details]
Patch v1

Clearing flags on attachment: 379354

Committed r250239: <https://trac.webkit.org/changeset/250239>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #4)
> Comment on attachment 379354 [details]
> Patch v1
> 
> In all these cases i would just use auto& rather than const auto&.

Why?  You don't feel `const` adds anything?

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> (In reply to Darin Adler from comment #4)
> > In all these cases i would just use auto& rather than const auto&.
> 
> Why?  You don't feel `const` adds anything?

Generally speaking we could put const in many more places in our C++ on local variables and non-reference arguments. In all those cases the benefit is similar: it will catch mistakes where we accidentally modify something that we don't intend to. And in all those cases the cost is similar: makes the code more verbose. In other languages like Swift, where it's easier to define things that are never modified (let vs. var) programmers typically make the other choice. But const in C++ is not the default so we generally don’t use it for simple cases like that. I kind of wish the default was reversed, but this is the language we have.

Here’s an example: Let’s say I am getting a pointer to an object and I plan not to use any non-const functions on it. Typically we don’t use const in a case like that:

    Element* focusedElement = document->focusedElement();
    if (focusedElement && focusedElement->isHappy() && focusedElement->isPatient())

We could add additional const, but we don’t because the benefit is small. It could be written like this:

    const Element* const focusedElement = document->focusedElement();
    if (focusedElement && focusedElement->isHappy() && focusedElement->isPatient())

But typically it’s not.

In the future I hope that most range-based for loops don't mention the type at all, and automatically use a type that won't cause any copying or other changes (like upcasting). Adding this to the language is proposed as N3994 <http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n3994.htm> and <http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n3853.htm> and I hope will eventually get into C++. The most generic way to do this safely is to use "auto&&", but in most cases "auto&" works fine and it's a little bit less exotic looking. Read the rationale in the N3853 document. Until this is available, when I read range-based for statements I see "auto&" and "auto&&" as "almost always right" and also brief. And I see "auto" as "almost always wrong" but also brief.

One other issue: in some of the cases where we are using "const auto&", the expression on the right side is already const. Typing "const auto&" in a case like this communicates "we will not modify the elements in this loop" but it has no effect on how the code is treated.