1324547 - ensure we're not needlessly copying things in ranged for loops

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Nathan Froyd [:froydnj]]

e.g. along the lines of:

https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/rangedforcopy.cxx

Quote:

// Check that we're not unnecessarily copying variables in a range based for loop
// e.g. "for (OUString a: aList)" results in a copy of each string being made,
// whereas "for (const OUString& a: aList)" does not.

The whole directory there is full of some interesting things that could be ported over to our static analyses.  Some of the plugins there are intended to be run over the whole codebase from time to time and then filtered through a post-processing step (kind of a poor man's LTO).

Comment 1

[Nathan Froyd [:froydnj]]

Is this something you'd be interested in doing?

Comment 2

[Andi [:andi]]

Sure, I think i have time for this at the beginning of next next year.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Looks like clang tidy has something for this:
http://clang.llvm.org/extra/clang-tidy/checks/performance-for-range-copy.html

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1324547 - ensure we're not needlessly copying things in ranged for loop

Review commit: https://reviewboard.mozilla.org/r/100228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100228/

Comment 5

[Sylvestre Ledru [:Sylvestre]]

It found only three occurrences only in tests:
https://reviewboard.mozilla.org/r/100228
So, either the clang-tidy is limited in term of detection or our code is good (I guess this is the first :p)

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> It found only three occurrences only in tests:
> https://reviewboard.mozilla.org/r/100228
> So, either the clang-tidy is limited in term of detection or our code is
> good (I guess this is the first :p)

I went through and fixed a couple other instances a while back via manually grepping and code examination. :p

I wish we required |const auto&| for those changes, but *shrug*.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Do you have example of those? Thanks

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Do you have example of those? Thanks

Bug 1287785.  I thought there were others somewhere in dom/media/, but a quick grep through my commits for 2016 doesn't show anything.

Comment 9

[Andi [:andi]]

This checker is pretty easy to adopt into our code, i think we will have a better view after we will have it in place. Basically what it needs to be done is to create a matcher of CXXForRangeStmt and examine the var declaration from the loop variable to be reference or pointer, otherwise assert an error message. I ill do this.

Comment 10

[Andi [:andi]]

This is the way how it's implemented using clang-tidy infrastructure, it can be easily adopted into our clang-plugin schematics:

https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/performance/ForRangeCopyCheck.cpp

This implementation is better since it avoids triggering the callback on ranged loops disregarding the loop variable qualification type.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

I think it should be fine if the object is a small POD type such as uint32_t, in addition to pointers and references.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8820785 [details]
Bug 1324547 - ensure we're not needlessly copying things in ranged for loop

https://reviewboard.mozilla.org/r/100228/#review102322

OK with the instances modified to `const auto&`, as below.  Thanks!

::: dom/plugins/base/nsPluginHost.cpp:1582
(Diff revision 1)
>  
>    RefPtr<nsFakePluginTag> newTag;
>    nsresult rv = nsFakePluginTag::Create(initDictionary, getter_AddRefs(newTag));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  for (auto existingTag : mFakePlugins) {
> +  for (const auto existingTag : mFakePlugins) {

I would feel way better about reading this if it was `const auto&`.  I know I went and thought about it after writing comment 6, but I've completely forgotten what the rationale is!  Writing it `const auto&` is also a little more in tune with the current/emerging style of `const auto&&`, but not nearly so radical.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8820785 [details]
Bug 1324547 - ensure we're not needlessly copying things in ranged for loop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100228/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12b902bdace0
ensure we're not needlessly copying things in ranged for loop r=froydnj

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/12b902bdace0

Comment 16

[Andi [:andi]]

This has been fixed since now our analysis has this checker as default:

https://dxr.mozilla.org/mozilla-central/source/tools/clang-tidy/config.yaml#46

Also this checker tries to determine if using reference or pointer is more efficient for some types by using: isExpensiveToCopy, see: https://clang.llvm.org/extra/doxygen/namespaceclang_1_1tidy_1_1utils_1_1type__traits.html#aa26ca62a658038b6d4d15da8b4e5e965

Comment 17

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.