202096 2019-09-22 20:33:03 -0700 clang-tidy: Fix unnecessary copy/ref churn of for loop variables in WebKit 2019-09-25 09:56:12 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=202069 https://bugs.webkit.org/show_bug.cgi?id=202090 InRadar P2 Normal --- 1 ddkilzer ddkilzer commit-queue darin dino megan_gardner simon.fraser thorton webkit-bug-importer oldest_to_newest 1573227 0 ddkilzer 2019-09-22 20:33:03 -0700 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 & 1573228 1 379354 ddkilzer 2019-09-22 20:37:34 -0700 Created attachment 379354 Patch v1 1573229 2 webkit-bug-importer 2019-09-22 20:38:02 -0700 <rdar://problem/55609903> 1573285 3 ddkilzer 2019-09-23 06:43:16 -0700 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> 1573316 4 379354 darin 2019-09-23 09:08:47 -0700 Comment on attachment 379354 Patch v1 In all these cases i would just use auto& rather than const auto&. 1573334 5 379354 commit-queue 2019-09-23 10:05:01 -0700 Comment on attachment 379354 Patch v1 Clearing flags on attachment: 379354 Committed r250239: <https://trac.webkit.org/changeset/250239> 1573335 6 commit-queue 2019-09-23 10:05:02 -0700 All reviewed patches have been landed. Closing bug. 1573464 7 ddkilzer 2019-09-23 16:13:11 -0700 (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? 1574089 8 darin 2019-09-25 09:56:12 -0700 (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. 379354 2019-09-22 20:37:34 -0700 2019-09-23 10:05:01 -0700 Patch v1 bug-202096-20190922203733.patch text/plain 4447 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ5OTUwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDFkNmU2OWEwNDM4MjRlMzc0 MmIxZDg0NTUwZGViNzQzNTBhOWJiNzEuLmI5ZjY5OTI0NjAxNDlkYzZlOTM1OTM5ZGYwNmYzN2Vl MDkzZmYxZTMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTktMDktMjIgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBjbGFuZy10aWR5OiBGaXggdW5u ZWNlc3NhcnkgY29weS9yZWYgY2h1cm4gb2YgZm9yIGxvb3AgdmFyaWFibGVzIGluIFdlYktpdAor ICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMjAyMDk2PgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeCB1bndhbnRlZCBjb3B5aW5nL3JlZiBj aHVybiBvZiBsb29wIHZhcmlhYmxlcyBieSBtYWtpbmcgdGhlbQorICAgICAgICBjb25zdCByZWZl cmVuY2VzLgorCisgICAgICAgICogTmV0d29ya1Byb2Nlc3MvQ2xhc3NpZmllci9SZXNvdXJjZUxv YWRTdGF0aXN0aWNzRGF0YWJhc2VTdG9yZS5jcHA6CisgICAgICAgIChXZWJLaXQ6OmRvbWFpbnNU b1N0cmluZyk6CisgICAgICAgICogVUlQcm9jZXNzL2lvcy9XS0FjdGlvblNoZWV0QXNzaXN0YW50 Lm1tOgorICAgICAgICAoLVtXS0FjdGlvblNoZWV0QXNzaXN0YW50IHByZXNlbnRhdGlvblJlY3RG b3JFbGVtZW50VXNpbmdDbG9zZXN0SW5kaWNhdGVkUmVjdF0pOgorICAgICAgICAqIFVJUHJvY2Vz cy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLm1tOgorICAgICAgICAoLVtXS0NvbnRlbnRW aWV3IF9zaW5nbGVUYXBEaWRSZXNldDpdKToKKyAgICAgICAgKiBVSVByb2Nlc3MvaW9zL1dlYkRh dGFMaXN0U3VnZ2VzdGlvbnNEcm9wZG93bklPUy5tbToKKyAgICAgICAgKC1bV0tEYXRhTGlzdFN1 Z2dlc3Rpb25zQ29udHJvbCB0ZXh0U3VnZ2VzdGlvbnNdKToKKwogMjAxOS0wOS0xNyAgRGVhbiBK YWNrc29uICA8ZGlub0BhcHBsZS5jb20+CiAKICAgICAgICAgUmVtb3ZlIHRoZSAiU2hvdyBMaW5r IFByZXZpZXdzIiBhbmQgIkhpZGUgTGluayBQcmV2aWV3cyIgYWN0aW9uIG1lbnVzIGluIHRoZSBw cmV2aWV3IHBsYXR0ZXIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3Mv Q2xhc3NpZmllci9SZXNvdXJjZUxvYWRTdGF0aXN0aWNzRGF0YWJhc2VTdG9yZS5jcHAgYi9Tb3Vy Y2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL0NsYXNzaWZpZXIvUmVzb3VyY2VMb2FkU3RhdGlzdGlj c0RhdGFiYXNlU3RvcmUuY3BwCmluZGV4IGQ0ZWM4NGM3YzdkMmUxNWI0ZTc4YzAyOWJhMTZmMjI2 NjdkYzIzNjYuLjdlMWYwYWNmM2VkZjhlN2ZhZTk1YjRhY2EzY2FmZGMyMTdkOTM0ZjQgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvQ2xhc3NpZmllci9SZXNvdXJjZUxv YWRTdGF0aXN0aWNzRGF0YWJhc2VTdG9yZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3Jr UHJvY2Vzcy9DbGFzc2lmaWVyL1Jlc291cmNlTG9hZFN0YXRpc3RpY3NEYXRhYmFzZVN0b3JlLmNw cApAQCAtNTExLDcgKzUxMSw3IEBAIHZvaWQgUmVzb3VyY2VMb2FkU3RhdGlzdGljc0RhdGFiYXNl U3RvcmU6OmNhbGN1bGF0ZUFuZFN1Ym1pdFRlbGVtZXRyeSgpIGNvbnN0CiBzdGF0aWMgU3RyaW5n IGRvbWFpbnNUb1N0cmluZyhjb25zdCBIYXNoU2V0PFJlZ2lzdHJhYmxlRG9tYWluPiYgZG9tYWlu cykKIHsKICAgICBTdHJpbmdCdWlsZGVyIGJ1aWxkZXI7Ci0gICAgZm9yIChhdXRvIGRvbWFpbk5h bWUgOiBkb21haW5zKSB7CisgICAgZm9yIChjb25zdCBhdXRvJiBkb21haW5OYW1lIDogZG9tYWlu cykgewogICAgICAgICBpZiAoIWJ1aWxkZXIuaXNFbXB0eSgpKQogICAgICAgICAgICAgYnVpbGRl ci5hcHBlbmRMaXRlcmFsKCIsICIpOwogICAgICAgICBidWlsZGVyLmFwcGVuZCgnIicpOwpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvaW9zL1dLQWN0aW9uU2hlZXRBc3Npc3Rh bnQubW0gYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9pb3MvV0tBY3Rpb25TaGVldEFzc2lzdGFu dC5tbQppbmRleCA5MTg0YjNmNzBmY2U5OGQzMDY2OGIzM2IwMDllNzAyODY4NGVjNjA5Li5hN2Zh NTE3ZWNiMjAwM2I3YjViMjFkZTQyYmUxOWY0OTA0NzA2MGQ2IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L1VJUHJvY2Vzcy9pb3MvV0tBY3Rpb25TaGVldEFzc2lzdGFudC5tbQorKysgYi9Tb3Vy Y2UvV2ViS2l0L1VJUHJvY2Vzcy9pb3MvV0tBY3Rpb25TaGVldEFzc2lzdGFudC5tbQpAQCAtMjEy LDcgKzIxMiw3IEBAIC0gKENHUmVjdClwcmVzZW50YXRpb25SZWN0Rm9yRWxlbWVudFVzaW5nQ2xv c2VzdEluZGljYXRlZFJlY3QKICAgICAgICAgaW5kaWNhdGVkUmVjdHMuYXBwZW5kKHJlY3QpOwog ICAgIH0KIAotICAgIGZvciAoYXV0byBwYXRoIDogV2ViQ29yZTo6UGF0aFV0aWxpdGllczo6cGF0 aHNXaXRoU2hyaW5rV3JhcHBlZFJlY3RzKGluZGljYXRlZFJlY3RzLCAwKSkgeworICAgIGZvciAo Y29uc3QgYXV0byYgcGF0aCA6IFdlYkNvcmU6OlBhdGhVdGlsaXRpZXM6OnBhdGhzV2l0aFNocmlu a1dyYXBwZWRSZWN0cyhpbmRpY2F0ZWRSZWN0cywgMCkpIHsKICAgICAgICAgYXV0byBib3VuZGlu Z1JlY3QgPSBwYXRoLmZhc3RCb3VuZGluZ1JlY3QoKTsKICAgICAgICAgaWYgKGJvdW5kaW5nUmVj dC5jb250YWlucyh0b3VjaExvY2F0aW9uKSkKICAgICAgICAgICAgIHJldHVybiBDR1JlY3RJbnNl dChbdmlldyBjb252ZXJ0UmVjdDooQ0dSZWN0KWJvdW5kaW5nUmVjdCBmcm9tVmlldzpfdmlldy5n ZXRBdXRvcmVsZWFzZWQoKV0sIC1wcmVzZW50YXRpb25FbGVtZW50UmVjdFBhZGRpbmcsIC1wcmVz ZW50YXRpb25FbGVtZW50UmVjdFBhZGRpbmcpOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9V SVByb2Nlc3MvaW9zL1dLQ29udGVudFZpZXdJbnRlcmFjdGlvbi5tbSBiL1NvdXJjZS9XZWJLaXQv VUlQcm9jZXNzL2lvcy9XS0NvbnRlbnRWaWV3SW50ZXJhY3Rpb24ubW0KaW5kZXggM2U5MjYyNzcy MzNkZDY1MTllMmY2NTQ0NDI1ZDNlM2FjZjc0ODYxZi4uZTc0ZmFiNjJhZmYyYmY1NGI3YjVjZDJm NGNmMTg4MTcxMzcxNTc1MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvaW9z L1dLQ29udGVudFZpZXdJbnRlcmFjdGlvbi5tbQorKysgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vz cy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLm1tCkBAIC0yNDg4LDcgKzI0ODgsNyBAQCAt ICh2b2lkKV9zaW5nbGVUYXBEaWRSZXNldDooVUlUYXBHZXN0dXJlUmVjb2duaXplciAqKWdlc3R1 cmVSZWNvZ25pemVyCiAgICAgfQogI2VuZGlmCiAgICAgYXV0byBhY3Rpb25zVG9QZXJmb3JtID0g c3RkOjpleGNoYW5nZShfYWN0aW9uc1RvUGVyZm9ybUFmdGVyUmVzZXR0aW5nU2luZ2xlVGFwR2Vz dHVyZVJlY29nbml6ZXIsIHsgfSk7Ci0gICAgZm9yIChhdXRvIGFjdGlvbiA6IGFjdGlvbnNUb1Bl cmZvcm0pCisgICAgZm9yIChjb25zdCBhdXRvJiBhY3Rpb24gOiBhY3Rpb25zVG9QZXJmb3JtKQog ICAgICAgICBhY3Rpb24oKTsKIH0KIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nl c3MvaW9zL1dlYkRhdGFMaXN0U3VnZ2VzdGlvbnNEcm9wZG93bklPUy5tbSBiL1NvdXJjZS9XZWJL aXQvVUlQcm9jZXNzL2lvcy9XZWJEYXRhTGlzdFN1Z2dlc3Rpb25zRHJvcGRvd25JT1MubW0KaW5k ZXggZDExYTk2ZDVkMmYyMWU1YmE5OTVlMDY2ZGQ4N2YwMjAyNDEzYmUwOS4uYjlkMzMwNjc5NzUw MTQxZjVjNGVkYmNmN2Y1NDYwYTkwOTMwZGRmMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9V SVByb2Nlc3MvaW9zL1dlYkRhdGFMaXN0U3VnZ2VzdGlvbnNEcm9wZG93bklPUy5tbQorKysgYi9T b3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9pb3MvV2ViRGF0YUxpc3RTdWdnZXN0aW9uc0Ryb3Bkb3du SU9TLm1tCkBAIC0xNjksNyArMTY5LDcgQEAgLSAoTlNBcnJheTxXS0RhdGFMaXN0VGV4dFN1Z2dl c3Rpb24gKj4gKil0ZXh0U3VnZ2VzdGlvbnMKIHsKICAgICBOU011dGFibGVBcnJheSAqc3VnZ2Vz dGlvbnMgPSBbTlNNdXRhYmxlQXJyYXkgYXJyYXldOwogCi0gICAgZm9yIChhdXRvIHN1Z2dlc3Rp b24gOiBfc3VnZ2VzdGlvbnMpIHsKKyAgICBmb3IgKGNvbnN0IGF1dG8mIHN1Z2dlc3Rpb24gOiBf c3VnZ2VzdGlvbnMpIHsKICAgICAgICAgW3N1Z2dlc3Rpb25zIGFkZE9iamVjdDpbV0tEYXRhTGlz dFRleHRTdWdnZXN0aW9uIHRleHRTdWdnZXN0aW9uV2l0aElucHV0VGV4dDpzdWdnZXN0aW9uXV07 CiAgICAgICAgIGlmIChzdWdnZXN0aW9ucy5jb3VudCA9PSAzKQogICAgICAgICAgICAgYnJlYWs7 Cg==