1126813 - analysis to detect for (auto i : container) and suggest something more suitable

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Nathan Froyd [:froydnj]]

Range-based for loops are very convenient, but the easy way to write them:

  for (auto i : container) { ... }

will invoke copy-constructors unnecessarily.  That's almost always not what we want.  We should have a static analysis that figures out if we'd be invoking (non-trivial?) copy constructors in range-based for loops and disallows them, requiring:

  for (auto& i : container) { ... }

instead.  I know bent prefers (and maybe enforces in reviews?) |auto&| or |auto*| to make reference/pointer-ness explicit.  Maybe we should adopt that for the style guide...?

Comment 1

[Biru [:poiru]]

Some future version of Clang will contain a warning for this: http://reviews.llvm.org/rL234804

Comment 2

[(no longer active)]

Should we just turn on -Werror=range-loop-analysis?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #2)
> Should we just turn on -Werror=range-loop-analysis?

Assuming I understand the comments in the commit (hooray for non-existent documentation), I think this is what we want.

Comment 5

[(no longer active)]

Attachment: Turn on the -Wrange-loop-analysis warning if available

Comment 6

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=766b658c4c22

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8657439 [details] [diff] [review]
Turn on the -Wrange-loop-analysis warning if available

Review of attachment 8657439 [details] [diff] [review]:
-----------------------------------------------------------------

Unrelated to this change, it looks like we have some duplication of code between the 2 configures. Since this code is related to code sanity and since I believe Gecko and SpiderMonkey share a unified convention now, we may want to follow-up by consolidating this code into a standalone m4 file.

Comment 8

[(no longer active)]

The warning flags are quite different, so I don't think that would result in any code sharing.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cac9265651a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cac9265651a