Closed Bug 1126813 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: froydnj, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

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...?
Some future version of Clang will contain a warning for this: http://reviews.llvm.org/rL234804
Should we just turn on -Werror=range-loop-analysis?
Flags: needinfo?(nfroyd)
Duplicate of this bug: 1123145
(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.
Flags: needinfo?(nfroyd)
Flags: needinfo?(ehsan)
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.
Attachment #8657439 - Flags: review?(gps) → review+
The warning flags are quite different, so I don't think that would result in any code sharing.
https://hg.mozilla.org/mozilla-central/rev/0cac9265651a
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.