All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: froydnj, Assigned: Ehsan)

Tracking

unspecified
mozilla43

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 2

3 years ago
Should we just turn on -Werror=range-loop-analysis?
Flags: needinfo?(nfroyd)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1123145
(Reporter)

Comment 4

3 years ago
(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)
(Assignee)

Updated

3 years ago
Flags: needinfo?(ehsan)
(Assignee)

Comment 5

3 years ago
Created attachment 8657439 [details] [diff] [review]
Turn on the -Wrange-loop-analysis warning if available
Attachment #8657439 - Flags: review?(gps)

Comment 7

3 years ago
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+
(Assignee)

Comment 8

3 years ago
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
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.