Add -Wrange-loop-analysis to warn when a range-based for-loop is creating a copy.

[rtrieu]

Summary

This warning will trigger on loop variables that make copies in a range-based for-loop. The three cases this warning catches are:

1. for (const Foo &x : Foos), where the range Foos does not return a copy. This warning will suggest using the non-reference type so the copy is obvious.
2. for (const Foo x : Foos), where the range Foos does return a reference, but is copied into x. This warning will suggest using the reference type to prevent a copy from being made.
3. for (const Bar &x : Foos), where Bar is constructed from Foo. In this case, suggest using the non-reference "const Bar" to indicate a copy is intended to be made, or "const Foo &" to prevent a copy from being made.

-Wrange-loop-analysis is being added as a sub-group to -Wloop-analysis. The previous warnings there are moved to -Wfor-loop-analysis. While the warnings are currently split along statement types, a finer grain division of warnings may be needed.

[dblaikie]

lib/Sema/SemaStmt.cpp

2361 ↗

(On Diff #10476)

I think Alp's been trying to move to using an "isIgnored" function to do this test for some reason - I haven't looked into the motivation, but it might be something to check if you should be consistent with.

2373 ↗

(On Diff #10476)

What are the cases where a for loop has no loop variable? (or the loop variable has no init, down on line 2382)

2386 ↗

(On Diff #10476)

Might be worth splitting out these cases as separate functions, maybe?

2449 ↗

(On Diff #10476)

The header comment for this function doesn't mention this case of Foo v Bar, it just mentions:

Foo foos[5];
for (const Foo f : foos);

->

for (const Foo &f : foos);

And the implementation looks like that's the intent of the code. Is the comment out of date/wrong?

2452 ↗

(On Diff #10476)

Not sure if it'd be more readable, but you could just assign:

MakeCopy = CE->getConstructor()->isCopyConstructor();

2455 ↗

(On Diff #10476)

Same assignment possible here

2467 ↗

(On Diff #10476)

return here seems a bit unnecessary (and/or as above, this whole case might be nice as a separate function for readability)

test/SemaCXX/warn-range-loop-analysis.cpp

14 ↗

(On Diff #10476)

I'm not sure if this merits a change to the test code, but this is a bit surprising/out-of-character to have a container of T return T by value from its iterator's op*. (usually it would return a reference to T)

41 ↗

(On Diff #10476)

I think this is going to have a pretty high false positive rate... do you have some numbers?

46 ↗

(On Diff #10476)

If these test functions provide logical grouping, perhaps they could be named based on that grouping, or have a comment describing it?

47 ↗

(On Diff #10476)

any reason for the typedef here, but not in test0 above?

50 ↗

(On Diff #10476)

Looks the same as the previous tests? (line 33, specifically)

If this test is to ensure that a container wrapped in a typedef is handled correctly - what kind of problems did you have with this situation that you had to handle/correct for?

73 ↗

(On Diff #10476)

I worry about people creating expensive copies this way, but maybe that'll need to be a clang-tidy check rather than a warning.

83 ↗

(On Diff #10476)

Same questions as above

[rtrieu]

lib/Sema/SemaStmt.cpp

2361 ↗

(On Diff #10476)

Phabricator doesn't show it, but the revision this patch was based on preceded Alp's patch, so using the isIgnored wasn't an option at the time. I have rebased to a later revision and now use it.

2373 ↗

(On Diff #10476)

There is not guarantee that this is a well-formed for loop. It is possible that ill-formed code created a less that perfect AST, which we should protect against.

2386 ↗

(On Diff #10476)

Done.

2449 ↗

(On Diff #10476)

Confusing comment removed.

2452 ↗

(On Diff #10476)

Used an early return in the separate function instead of the MakeCopy variable.

2455 ↗

(On Diff #10476)

Same as above.

2467 ↗

(On Diff #10476)

Removed.

test/SemaCXX/warn-range-loop-analysis.cpp

14 ↗

(On Diff #10476)

Using two separate containers now. Container uses reference returning iterators. ContainerNonReference uses non-reference returning iterators.

41 ↗

(On Diff #10476)

What is your idea of a false positive?

46 ↗

(On Diff #10476)

Test comments added above.

47 ↗

(On Diff #10476)

Removed typedef. Relevant information moved to test comments above.

50 ↗

(On Diff #10476)

test0 checks the entire warning and note messages while the remainder of the tests focuses on thoroughly testings the different combinations of types.

Typedefs have been removed as nothing tests them.

73 ↗

(On Diff #10476)

Since there are only two forms of const loop variables, this one must be the one accepted for copies. Otherwise, we would have to suggest something like:

for (const int x : A) {
  const Bar y(x);
}

in order to silence the warning.

83 ↗

(On Diff #10476)

Same answers as above.

[dblaikie]

lib/Sema/SemaStmt.cpp

2354 ↗

(On Diff #10909)

This check is already performed in the caller - I think it's OK to just omit it entirely here (and in DiagnoseForRangeConstVariableCopies) but if you think it's better for readability, perhaps it could be an assert instead?

test/SemaCXX/warn-range-loop-analysis.cpp

41 ↗

(On Diff #10476)

Users are taught to pass ints to functions by value, rather than reference, to avoid the indirection overhead. The same logic applies (to a degree) to for loops. It's not unreasonable to write "for (const int i : my_vec_of_ints)".

I don't think this case is likely to be a good thing to warn on and we may need a heuristic of "if a type is 'large' enough (or has a non-trivial copy), warn when a copy is made". StringRef would be a similar situation - I can imagine lots of people (I bet we've got a few in Clang/LLVM already) looping over StringRef's by value.

Do you have some stats on running this warning in its current form over LLVM/Clang? (and Google, for that matter, or any other substantial codebase).

Looking at other test cases, this wouldn't warn if the "const" was removed? Perhaps we just need a fix to disregard cv qualifiers, then?

50 ↗

(On Diff #10476)

Struck me as a bit strange to have the duplication, but I guess you chose it for consistency? (so this tests each case in turn, doesn't have strangely missing cases from the pattern just because they were covered elsewhere?) I'm OK with that.

73 ↗

(On Diff #10476)

Honestly, I think that might be worthwhile - but would need stats to understand it better. And there's no harm in the first version of the warning being conservative - more aggressive diagnosis can be done incrementally with relevant data.

[rtrieu]

test/SemaCXX/warn-range-loop-analysis.cpp

41 ↗

(On Diff #10476)

LLVM/Clang has a total of 5 warnings emitted for this warning. (Where changing the variable to reference type will prevent a copy.) Two are pairs from maps, one is a CXXBaseSpecifier, one is QualType, and one is a ParmVarDecl pointer. From your description, the QualType and Decl pointer are both small and trivially copyable so they should be exempt from this warning, so a 40% drop in cases with your suggested filter.

Applying the same filter to results from Google, the drop is closer to 70%. The filter is to ignore trivially copyable types which are 2x pointer size or smaller.

The const is a good indication that the variable is read-only, so we can suggest using a const-reference as the cheaper option. Without the const, we don't know if the use in the loop was intended only for the loop copy or to change the object in the container as well.

[dblaikie]

test/SemaCXX/warn-range-loop-analysis.cpp

41 ↗

(On Diff #10476)

You mention a 70% drop in Google - but what does a random sample of the remaining (and omitted) cases look like?

I'm trying to understand if this warning is good or not, as-is, which means we need a reasonable false positive % estimate/number to decide whether it needs to be tweaked further to reduce false positives (that's the main thing - if it's got lots of false negatives that we can gain back, that can be done in separate patches, but going in-tree with high false positive rates is generally frowned upon)

Perhaps I'm missing something where you described this already.

[rtrieu]

Don't warn when POD types are copied. It's likely that some of the larger POD types should be warned on, but a reasonable limit is needed since copies of smaller POD types is a regular occurrence.

[dblaikie]

You could probably be a bit narrower than POD types - probably just
types with trivial copy constructors. But for now "all POD types"
shouldn't have any false positives, only false negatives - so perhaps
leave it that way with a FIXME Describing a narrower check for small
types (small to be defined/discovered) with trivial copy construction.

[rtrieu]

Update patch to be based on a newer revision.

[dblaikie]

In D4169#68650, @dblaikie wrote:

You could probably be a bit narrower than POD types - probably just
types with trivial copy constructors. But for now "all POD types"
shouldn't have any false positives, only false negatives - so perhaps
leave it that way with a FIXME Describing a narrower check for small
types (small to be defined/discovered) with trivial copy construction.

Have you addressed these suggestions?

I don't recall where this was all left, exactly.

[rtrieu]

In D4169#131597, @dblaikie wrote:

In D4169#68650, @dblaikie wrote:

You could probably be a bit narrower than POD types - probably just
types with trivial copy constructors. But for now "all POD types"
shouldn't have any false positives, only false negatives - so perhaps
leave it that way with a FIXME Describing a narrower check for small
types (small to be defined/discovered) with trivial copy construction.

Have you addressed these suggestions?

I don't recall where this was all left, exactly.

Currently, all POD types for copies are ignored. There is a comment to only ignore types with trivial constructors and to figure out a proper size for small in a future revision.

[rtrieu]

Results from running the warning on LLVM projects. The actual errors are at the end. In total, this warning was triggered 22 times. The breakdown is:

7 - for (const Foo &x : Foos), where the range Foos only return a copy. Suggest using the non-reference type so the copy is obvious.
5 - for (const Foo x : Foos), where the range Foos does return a reference, but is copied into x. Suggest using the reference type to prevent a copy from being made.
10 - for (const Bar &x : Foos), where Bar is constructed from Foo. Suggest using the non-reference "const Bar" to indicate a copy is intended to be made, or "const Foo &" to prevent a copy from being made.

llvm/lib/Analysis/RegionPass.cpp:197:22: error: loop variable 'BB' is always a copy because the range of type 'llvm::iterator_range<llvm::RegionBase<llvm::RegionTraits<llvm::Function> >::block_iterator_wrapper<false> >' does not return a reference
llvm/lib/Analysis/RegionPass.cpp:197:10: note: use non-reference type 'llvm::BasicBlock *const'
llvm/lib/Analysis/RegionPrinter.cpp:126:22: error: loop variable 'BB' is always a copy because the range of type 'llvm::iterator_range<llvm::RegionBase<llvm::RegionTraits<llvm::Function> >::block_iterator_wrapper<true> >' does not return a reference
llvm/lib/Analysis/RegionPrinter.cpp:126:10: note: use non-reference type 'llvm::BasicBlock *const'
llvm/lib/MC/MCDwarf.cpp:814:19: error: loop variable 'sec' of type 'const std::pair<const llvm::MCSection *, std::pair<llvm::MCSymbol *, llvm::MCSymbol *> >' creates a copy from type 'const std::pair<const llvm::MCSection *, std::pair<llvm::MCSymbol *, llvm::MCSymbol *> >'
llvm/lib/MC/MCDwarf.cpp:814:8: note: use reference type 'const std::pair<const llvm::MCSection *, std::pair<llvm::MCSymbol *, llvm::MCSymbol *> > &' to prevent copying
llvm/lib/Object/COFFObjectFile.cpp:263:31: error: loop variable 'SymbI' has type 'const llvm::object::symbol_iterator &' but is initialized with type 'const llvm::object::SymbolRef' resulting in a copy
llvm/lib/Object/COFFObjectFile.cpp:263:8: note: use non-reference type 'const llvm::object::symbol_iterator' to keep the copy or type 'const llvm::object::SymbolRef &' to prevent copying
llvm/lib/Target/AArch64/AArch64CollectLOH.cpp:331:25: error: loop variable 'Entry' of type 'const llvm::detail::DenseMapPair<unsigned int, unsigned int>' creates a copy from type 'const llvm::detail::DenseMapPair<unsigned int, unsigned int>'
llvm/lib/Target/AArch64/AArch64CollectLOH.cpp:331:14: note: use reference type 'const llvm::detail::DenseMapPair<unsigned int, unsigned int> &' to prevent copying
llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:889:20: error: loop variable 'BB' is always a copy because the range of type 'llvm::iterator_range<llvm::RegionBase<llvm::RegionTraits<llvm::Function> >::block_iterator_wrapper<false> >' does not return a reference
llvm/lib/Transforms/Scalar/StructurizeCFG.cpp:889:8: note: use non-reference type 'llvm::BasicBlock *const'
llvm/tools/clang/lib/AST/MicrosoftMangle.cpp:1614:25: error: loop variable 'Arg' of type 'const clang::QualType' creates a copy from type 'const clang::QualType'
llvm/tools/clang/lib/AST/MicrosoftMangle.cpp:1614:10: note: use reference type 'const clang::QualType &' to prevent copying
llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:3439:19: error: loop variable 'I' of type 'const llvm::detail::DenseMapPair<const clang::Decl *, bool>' creates a copy from type 'const llvm::detail::DenseMapPair<const clang::Decl *, bool>'
llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:3439:8: note: use reference type 'const llvm::detail::DenseMapPair<const clang::Decl *, bool> &' to prevent copying
llvm/tools/clang/lib/CodeGen/CoverageMappingGen.cpp:945:22: error: loop variable 'Arg' is always a copy because the range of type 'llvm::iterator_range<clang::ConstExprIterator>' does not return a reference
llvm/tools/clang/lib/CodeGen/CoverageMappingGen.cpp:945:10: note: use non-reference type 'const clang::Expr *const'
llvm/tools/clang/lib/Sema/SemaLookup.cpp:3026:20: error: loop variable 'R' is always a copy because the range of type 'llvm::iterator_range<clang::DeclContext::all_lookups_iterator>' does not return a reference
llvm/tools/clang/lib/Sema/SemaLookup.cpp:3026:8: note: use non-reference type 'const llvm::MutableArrayRef<clang::NamedDecl *>'
llvm/tools/clang/lib/Sema/SemaTemplateInstantiate.cpp:1776:19: error: loop variable 'Base' of type 'const clang::CXXBaseSpecifier' creates a copy from type 'const clang::CXXBaseSpecifier'
llvm/tools/clang/lib/Sema/SemaTemplateInstantiate.cpp:1776:8: note: use reference type 'const clang::CXXBaseSpecifier &' to prevent copying
llvm/tools/clang/tools/extra/clang-rename/USRFindingAction.cpp:49:20: error: loop variable 'CtorDecl' is always a copy because the range of type 'llvm::iterator_range<clang::DeclContext::specific_decl_iterator<clang::CXXConstructorDecl> >' does not return a reference
llvm/tools/clang/tools/extra/clang-rename/USRFindingAction.cpp:49:8: note: use non-reference type 'clang::CXXConstructorDecl *const'
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:299:68: error: loop variable 'VFTableEntry' has type 'const std::pair<std::pair<StringRef, uint64_t>, StringRef> &' but is initialized with type 'std::pair<const std::pair<llvm::StringRef, unsigned long>, llvm::StringRef>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:299:8: note: use non-reference type 'const std::pair<std::pair<StringRef, uint64_t>, StringRef>' to keep the copy or type 'const std::pair<const std::pair<llvm::StringRef, unsigned long>, llvm::StringRef> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:306:58: error: loop variable 'VBTable' has type 'const std::pair<StringRef, ArrayRef<little32_t> > &' but is initialized with type 'std::pair<const llvm::StringRef, llvm::ArrayRef<llvm::support::detail::packed_endian_specific_integral<int, llvm::support::endianness::little, 1> > >' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:306:8: note: use non-reference type 'const std::pair<StringRef, ArrayRef<little32_t> >' to keep the copy or type 'const std::pair<const llvm::StringRef, llvm::ArrayRef<llvm::support::detail::packed_endian_specific_integral<int, llvm::support::endianness::little, 1> > > &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:314:59: error: loop variable 'COLPair' has type 'const std::pair<StringRef, CompleteObjectLocator> &' but is initialized with type 'std::pair<const llvm::StringRef, CompleteObjectLocator>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:314:8: note: use non-reference type 'const std::pair<StringRef, CompleteObjectLocator>' to keep the copy or type 'const std::pair<const llvm::StringRef, CompleteObjectLocator> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:323:62: error: loop variable 'CHDPair' has type 'const std::pair<StringRef, ClassHierarchyDescriptor> &' but is initialized with type 'std::pair<const llvm::StringRef, ClassHierarchyDescriptor>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:323:8: note: use non-reference type 'const std::pair<StringRef, ClassHierarchyDescriptor>' to keep the copy or type 'const std::pair<const llvm::StringRef, ClassHierarchyDescriptor> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:331:68: error: loop variable 'BCAEntry' has type 'const std::pair<std::pair<StringRef, uint64_t>, StringRef> &' but is initialized with type 'std::pair<const std::pair<llvm::StringRef, unsigned long>, llvm::StringRef>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:331:8: note: use non-reference type 'const std::pair<std::pair<StringRef, uint64_t>, StringRef>' to keep the copy or type 'const std::pair<const std::pair<llvm::StringRef, unsigned long>, llvm::StringRef> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:338:57: error: loop variable 'BCDPair' has type 'const std::pair<StringRef, BaseClassDescriptor> &' but is initialized with type 'std::pair<const llvm::StringRef, BaseClassDescriptor>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:338:8: note: use non-reference type 'const std::pair<StringRef, BaseClassDescriptor>' to keep the copy or type 'const std::pair<const llvm::StringRef, BaseClassDescriptor> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:349:52: error: loop variable 'TDPair' has type 'const std::pair<StringRef, TypeDescriptor> &' but is initialized with type 'std::pair<const llvm::StringRef, TypeDescriptor>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:349:8: note: use non-reference type 'const std::pair<StringRef, TypeDescriptor>' to keep the copy or type 'const std::pair<const llvm::StringRef, TypeDescriptor> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:359:68: error: loop variable 'VTTPair' has type 'const std::pair<std::pair<StringRef, uint64_t>, StringRef> &' but is initialized with type 'std::pair<const std::pair<llvm::StringRef, unsigned long>, llvm::StringRef>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:359:8: note: use non-reference type 'const std::pair<std::pair<StringRef, uint64_t>, StringRef>' to keep the copy or type 'const std::pair<const std::pair<llvm::StringRef, unsigned long>, llvm::StringRef> &' to prevent copying
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:366:47: error: loop variable 'TIPair' has type 'const std::pair<StringRef, StringRef> &' but is initialized with type 'std::pair<const llvm::StringRef, llvm::StringRef>' resulting in a copy
llvm/tools/llvm-vtabledump/llvm-vtabledump.cpp:366:8: note: use non-reference type 'const std::pair<StringRef, StringRef>' to keep the copy or type 'const std::pair<const llvm::StringRef, llvm::StringRef> &' to prevent copying
llvm/utils/TableGen/CodeGenRegisters.cpp:1773:24: error: loop variable 'SUI' is always a copy because the range of type 'const RegUnitList' (aka 'const SparseBitVector<>') does not return a reference
llvm/utils/TableGen/CodeGenRegisters.cpp:1773:12: note: use non-reference type 'const unsigned int'

[dblaikie]

lib/Sema/SemaStmt.cpp

2412 ↗

(On Diff #20808)

Avoid branch-to-unreachable, just uncoditionally cast<MaterializeTemporaryExpr> in the previous block (as an else, not an else-if)

2427 ↗

(On Diff #20808)

Prefer an assertion rather than branch-to-unreachable

(eg: change the else if to an else, then just use cast<CXXOperatorCallExpr> since it must be true)

test/SemaCXX/warn-range-loop-analysis.cpp

13 ↗

(On Diff #20808)

Maybe just have one Container class that takes the iterator element type as a parameter? (T, T&, etc) - essentially your ContainerNonReference (renamed), and just instantiate it with T& if you want the reference version.

17 ↗

(On Diff #20808)

Is it much hassle to just have the code work in such a way that it doesn't have errors?

23 ↗

(On Diff #20808)

I'd probably jsut make all these classes structs & skip the access modifiers - there's not much to hide/fence off in a test case.

87 ↗

(On Diff #20808)

We seem to have some varied opinions on whether to drop 'const' when removing a reference - is their diagnostic precedence in clang we can draw from where we already suggest dropping refs (to see if we also suggest dropping const there too)?

I seem to recall this coming up in some discussion about a clang-tidy fix recently (the pass-by-value fixit hint, which is very similar to this warning)

98 ↗

(On Diff #20808)

Having the comments /after/ the code in question is a bit hard to read - is that intentional? Could it be the other way around, with the comment coming before the code?

180 ↗

(On Diff #20808)

typo, 'no' rather than 'now'?

292 ↗

(On Diff #20808)

Is the array case in any way differently handled in the code itself? If not, I probably wouldn't bother testing them (here, or the previous 2 test functions)

[rtrieu]

lib/Sema/SemaStmt.cpp

2412 ↗

(On Diff #20808)

Done.

2427 ↗

(On Diff #20808)

Done.

test/SemaCXX/warn-range-loop-analysis.cpp

13 ↗

(On Diff #20808)

Done.

17 ↗

(On Diff #20808)

Yes, we could remove all the non-const reference bindings that fail. Each test tries to bind/assign a type 4 different ways, T, T&, const T, and const T&. Since the other type is convertible, binding to T& sometimes will fail. I think we should keep them for completeness.

23 ↗

(On Diff #20808)

Done.

87 ↗

(On Diff #20808)

I don't recall how we handle removing references in other cases. Here, the code author had marked to loop variable const, making the intent clear that the const should be kept.

98 ↗

(On Diff #20808)

The @ after the expected diagnostic can work both ways, using minus to refer to earlier lines and plus to refer to later lines. In grep, I found 130 files with @-NUM while there were 120 files with @+num.

180 ↗

(On Diff #20808)

Fixed.

292 ↗

(On Diff #20808)

The arrays use a pointer type as their iterator. The Container struct above used a user-defined iterator. The code path is slightly different for the two.

[dblaikie]

Optional feedback left (just restating stuff I already said, perhaps it sounds good to you, but don't feel like you have to agree with me on any of those). Otherwise looks good, commit away!

test/SemaCXX/warn-range-loop-analysis.cpp

17 ↗

(On Diff #20808)

Could we just leave a comment in, explaining that this particular combination is invalid? (that way each test is consistent/follows the same formula, but we just explain why this particular intersection isn't relevant/tested)

I'm just a little scared of semi-random note/error-ignorance in a test & how that might hide some other behavior, maybe... not sure. I'm willing to set aside my preference here, though - said my piece & leave it up to you.

87 ↗

(On Diff #20808)

I don't think the intent is that clear - it's pretty common to mark references const when you're not planning to mutate them (sometimes out of necessity, because an API only provides a const reference, so you have to follow suit) - but it's pretty rare to put const on local value variables.

I cc'd you on an an internal bug discussing similar behavior in clang-tidy checks for passing by value (I'm not sure if this clang-tidy check is public, the discussion just happened to be internal) which, by the sounds of things, does what I'm suggesting here, and drops const when dropping reference-ness.

I think that's probably the right thing to do.

98 ↗

(On Diff #20808)

Curious - that seems counter to the usual habit, when writing normal code, to put a comment before the code it applies to. I wouldn't mind breaking in that direction given that there's sufficient variation in the population (though, admittedly, my grepping showed up 173:134 in favor of trailing @- rather than leading @+.. *shrug*)

292 ↗

(On Diff #20808)

Ah, OK.

[rtrieu]

This has been committed with dblaikie's suggestions to drop the const when getting the non-reference type and commenting out the loops which produce errors in the test case.

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def KNRPromotedParameter : DiagGroup<"knr-promoted-parameter">;
 def : DiagGroup<"init-self">;
 def : DiagGroup<"inline">;
 def : DiagGroup<"invalid-pch">;
 def GNULabelsAsValue : DiagGroup<"gnu-label-as-value">;
 def LiteralRange : DiagGroup<"literal-range">;
 def LocalTypeTemplateArgs : DiagGroup<"local-type-template-args",
                                       [CXX98CompatLocalTypeTemplateArgs]>;
-def LoopAnalysis : DiagGroup<"loop-analysis">;
+def RangeLoopAnalysis : DiagGroup<"range-loop-analysis">;
+def ForLoopAnalysis : DiagGroup<"for-loop-analysis">;
+def LoopAnalysis : DiagGroup<"loop-analysis", [ForLoopAnalysis,
+                                               RangeLoopAnalysis]>;
 def MalformedWarningCheck : DiagGroup<"malformed-warning-check">;
 def Main : DiagGroup<"main">;
 def MainReturnType : DiagGroup<"main-return-type">;
 def MissingBraces : DiagGroup<"missing-braces">;
 def MissingDeclarations: DiagGroup<"missing-declarations">;
 def : DiagGroup<"missing-format-attribute">;
 def : DiagGroup<"missing-include-dirs">;
 def MissingNoreturn : DiagGroup<"missing-noreturn">;

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def note_entity_declared_at : Note<"%0 declared here">;
 def note_callee_decl : Note<"%0 declared here">;
 def note_defined_here : Note<"%0 defined here">;
 
 // For loop analysis
 def warn_variables_not_in_loop_body : Warning<
   "variable%select{s| %1|s %1 and %2|s %1, %2, and %3|s %1, %2, %3, and %4}0 "
   "used in loop condition not modified in loop body">,
-  InGroup<LoopAnalysis>, DefaultIgnore;
+  InGroup<ForLoopAnalysis>, DefaultIgnore;
 def warn_redundant_loop_iteration : Warning<
   "variable %0 is %select{decremented|incremented}1 both in the loop header "
   "and in the loop body">,
-  InGroup<LoopAnalysis>, DefaultIgnore;
+  InGroup<ForLoopAnalysis>, DefaultIgnore;
 def note_loop_iteration_here : Note<"%select{decremented|incremented}0 here">;
 
+def warn_for_range_const_reference_copy : Warning<
+  "loop variable %0 "
+  "%diff{has type $ but is initialized with type $"
+  "| is initialized with a value of a different type}1,2 resulting in a copy">,
+  InGroup<RangeLoopAnalysis>, DefaultIgnore;
+def note_use_type_or_non_reference : Note<
+  "use non-reference type %0 to keep the copy or type %1 to prevent copying">;
+def warn_for_range_variable_always_copy : Warning<
+  "loop variable %0 is always a copy because the range of type %1 does not "
+  "return a reference">,
+  InGroup<RangeLoopAnalysis>, DefaultIgnore;
+def note_use_non_reference_type : Note<"use non-reference type %0">;
+def warn_for_range_copy : Warning<
+  "loop variable %0 of type %1 creates a copy from type %2">,
+  InGroup<RangeLoopAnalysis>, DefaultIgnore;
+def note_use_reference_type : Note<"use reference type %0 to prevent copying">;
+
 def warn_duplicate_enum_values : Warning<
   "element %0 has been implicitly assigned %1 which another element has "
   "been assigned">, InGroup<DiagGroup<"duplicate-enum">>, DefaultIgnore;
 def note_duplicate_element : Note<"element %0 also has value %1">;
 
 // Absolute value functions
 def warn_unsigned_abs : Warning<
   "taking the absolute value of unsigned type %0 has no effect">,

cfe/trunk/lib/Sema/SemaStmt.cpp

StmtResult Sema::FinishObjCForCollectionStmt(Stmt *S, Stmt *B) {
   if (!S || !B)
     return StmtError();
   ObjCForCollectionStmt * ForStmt = cast<ObjCForCollectionStmt>(S);
 
   ForStmt->setBody(B);
   return S;
 }
 
+// Warn when the loop variable is a const reference that creates a copy.
+// Suggest using the non-reference type for copies.  If a copy can be prevented
+// suggest the const reference type that would do so.
+// For instance, given "for (const &Foo : Range)", suggest
+// "for (const Foo : Range)" to denote a copy is made for the loop.  If
+// possible, also suggest "for (const &Bar : Range)" if this type prevents
+// the copy altogether.
+static void DiagnoseForRangeReferenceVariableCopies(Sema &SemaRef,
+                                                    const VarDecl *VD,
+                                                    QualType RangeInitType) {
+  const Expr *InitExpr = VD->getInit();
+  if (!InitExpr)
+    return;
+
+  QualType VariableType = VD->getType();
+
+  const MaterializeTemporaryExpr *MTE =
+      dyn_cast<MaterializeTemporaryExpr>(InitExpr);
+
+  // No copy made.
+  if (!MTE)
+    return;
+
+  const Expr *E = MTE->GetTemporaryExpr()->IgnoreImpCasts();
+
+  // Searching for either UnaryOperator for dereference of a pointer or
+  // CXXOperatorCallExpr for handling iterators.
+  while (!isa<CXXOperatorCallExpr>(E) && !isa<UnaryOperator>(E)) {
+    if (const CXXConstructExpr *CCE = dyn_cast<CXXConstructExpr>(E)) {
+      E = CCE->getArg(0);
+    } else if (const CXXMemberCallExpr *Call = dyn_cast<CXXMemberCallExpr>(E)) {
+      const MemberExpr *ME = cast<MemberExpr>(Call->getCallee());
+      E = ME->getBase();
+    } else {
+      const MaterializeTemporaryExpr *MTE = cast<MaterializeTemporaryExpr>(E);
+      E = MTE->GetTemporaryExpr();
+    }
+    E = E->IgnoreImpCasts();
+  }
+
+  bool ReturnsReference = false;
+  if (isa<UnaryOperator>(E)) {
+    ReturnsReference = true;
+  } else {
+    const CXXOperatorCallExpr *Call = cast<CXXOperatorCallExpr>(E);
+    const FunctionDecl *FD = Call->getDirectCallee();
+    QualType ReturnType = FD->getReturnType();
+    ReturnsReference = ReturnType->isReferenceType();
+  }
+
+  if (ReturnsReference) {
+    // Loop variable creates a temporary.  Suggest either to go with
+    // non-reference loop variable to indiciate a copy is made, or
+    // the correct time to bind a const reference.
+    SemaRef.Diag(VD->getLocation(), diag::warn_for_range_const_reference_copy)
+        << VD << VariableType << E->getType();
+    QualType NonReferenceType = VariableType.getNonReferenceType();
+    NonReferenceType.removeLocalConst();
+    QualType NewReferenceType =
+        SemaRef.Context.getLValueReferenceType(E->getType().withConst());
+    SemaRef.Diag(VD->getLocStart(), diag::note_use_type_or_non_reference)
+        << NonReferenceType << NewReferenceType << VD->getSourceRange();
+  } else {
+    // The range always returns a copy, so a temporary is always created.
+    // Suggest removing the reference from the loop variable.
+    SemaRef.Diag(VD->getLocation(), diag::warn_for_range_variable_always_copy)
+        << VD << RangeInitType;
+    QualType NonReferenceType = VariableType.getNonReferenceType();
+    NonReferenceType.removeLocalConst();
+    SemaRef.Diag(VD->getLocStart(), diag::note_use_non_reference_type)
+        << NonReferenceType << VD->getSourceRange();
+  }
+}
+
+// Warns when the loop variable can be changed to a reference type to
+// prevent a copy.  For instance, if given "for (const Foo x : Range)" suggest
+// "for (const Foo &x : Range)" if this form does not make a copy.
+static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef,
+                                                const VarDecl *VD) {
+  const Expr *InitExpr = VD->getInit();
+  if (!InitExpr)
+    return;
+
+  QualType VariableType = VD->getType();
+
+  if (const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(InitExpr)) {
+    if (!CE->getConstructor()->isCopyConstructor())
+      return;
+  } else if (const CastExpr *CE = dyn_cast<CastExpr>(InitExpr)) {
+    if (CE->getCastKind() != CK_LValueToRValue)
+      return;
+  } else {
+    return;
+  }
+
+  // TODO: Determine a maximum size that a POD type can be before a diagnostic
+  // should be emitted.  Also, only ignore POD types with trivial copy
+  // constructors.
+  if (VariableType.isPODType(SemaRef.Context))
+    return;
+
+  // Suggest changing from a const variable to a const reference variable
+  // if doing so will prevent a copy.
+  SemaRef.Diag(VD->getLocation(), diag::warn_for_range_copy)
+      << VD << VariableType << InitExpr->getType();
+  SemaRef.Diag(VD->getLocStart(), diag::note_use_reference_type)
+      << SemaRef.Context.getLValueReferenceType(VariableType)
+      << VD->getSourceRange();
+}
+
+/// DiagnoseForRangeVariableCopies - Diagnose three cases and fixes for them.
+/// 1) for (const foo &x : foos) where foos only returns a copy.  Suggest
+///    using "const foo x" to show that a copy is made
+/// 2) for (const bar &x : foos) where bar is a temporary intialized by bar.
+///    Suggest either "const bar x" to keep the copying or "const foo& x" to
+///    prevent the copy.
+/// 3) for (const foo x : foos) where x is constructed from a reference foo.
+///    Suggest "const foo &x" to prevent the copy.
+static void DiagnoseForRangeVariableCopies(Sema &SemaRef,
+                                           const CXXForRangeStmt *ForStmt) {
+  if (SemaRef.Diags.isIgnored(diag::warn_for_range_const_reference_copy,
+                              ForStmt->getLocStart()) &&
+      SemaRef.Diags.isIgnored(diag::warn_for_range_variable_always_copy,
+                              ForStmt->getLocStart()) &&
+      SemaRef.Diags.isIgnored(diag::warn_for_range_copy,
+                              ForStmt->getLocStart())) {
+    return;
+  }
+
+  const VarDecl *VD = ForStmt->getLoopVariable();
+  if (!VD)
+    return;
+
+  QualType VariableType = VD->getType();
+
+  if (VariableType->isIncompleteType())
+    return;
+
+  const Expr *InitExpr = VD->getInit();
+  if (!InitExpr)
+    return;
+
+  if (VariableType->isReferenceType()) {
+    DiagnoseForRangeReferenceVariableCopies(SemaRef, VD,
+                                            ForStmt->getRangeInit()->getType());
+  } else if (VariableType.isConstQualified()) {
+    DiagnoseForRangeConstVariableCopies(SemaRef, VD);
+  }
+}
+
 /// FinishCXXForRangeStmt - Attach the body to a C++0x for-range statement.
 /// This is a separate step from ActOnCXXForRangeStmt because analysis of the
 /// body cannot be performed until after the type of the range variable is
 /// determined.
 StmtResult Sema::FinishCXXForRangeStmt(Stmt *S, Stmt *B) {
   if (!S || !B)
     return StmtError();
 
   if (isa<ObjCForCollectionStmt>(S))
     return FinishObjCForCollectionStmt(S, B);
 
   CXXForRangeStmt *ForStmt = cast<CXXForRangeStmt>(S);
   ForStmt->setBody(B);
 
   DiagnoseEmptyStmtBody(ForStmt->getRParenLoc(), B,
                         diag::warn_empty_range_based_for_body);
 
+  DiagnoseForRangeVariableCopies(*this, ForStmt);
+
   return S;
 }
 
 StmtResult Sema::ActOnGotoStmt(SourceLocation GotoLoc,
                                SourceLocation LabelLoc,
                                LabelDecl *TheDecl) {
   getCurFunction()->setHasBranchIntoScope();
   TheDecl->markUsed(Context);

cfe/trunk/test/SemaCXX/warn-range-loop-analysis.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wrange-loop-analysis -verify %s
+
+template <typename return_type>
+struct Iterator {
+  return_type operator*();
+  Iterator operator++();
+  bool operator!=(const Iterator);
+};
+
+template <typename T>
+struct Container {
+  typedef Iterator<T> I;
+
+  I begin();
+  I end();
+};
+
+struct Foo {};
+struct Bar {
+  Bar(Foo);
+  Bar(int);
+  operator int();
+};
+
+// Testing notes:
+// test0 checks that the full text of the warnings and notes is correct.  The
+//   rest of the tests checks a smaller portion of the text.
+// test1-6 are set in pairs, the odd numbers are the non-reference returning
+//   versions of the even numbers.
+// test7-9 use an array instead of a range object
+// tests use all four versions of the loop varaible, const &T, const T, T&, and
+//   T.  Versions producing errors and are commented out.
+//
+// Conversion chart:
+//   double <=> int
+//   int    <=> Bar
+//   double  => Bar
+//   Foo     => Bar
+//
+// Conversions during tests:
+// test1-2
+//   int => int
+//   int => double
+//   int => Bar
+// test3-4
+//   Bar => Bar
+//   Bar => int
+// test5-6
+//   Foo => Bar
+// test7
+//   double => double
+//   double => int
+//   double => Bar
+// test8
+//   Foo => Foo
+//   Foo => Bar
+// test9
+//   Bar => Bar
+//   Bar => int
+
+void test0() {
+  Container<int> int_non_ref_container;
+  Container<int&> int_container;
+  Container<Bar&> bar_container;
+
+  for (const int &x : int_non_ref_container) {}
+  // expected-warning@-1 {{loop variable 'x' is always a copy because the range of type 'Container<int>' does not return a reference}}
+  // expected-note@-2 {{use non-reference type 'int'}}
+
+  for (const double &x : int_container) {}
+  // expected-warning@-1 {{loop variable 'x' has type 'const double &' but is initialized with type 'int' resulting in a copy}}
+  // expected-note@-2 {{use non-reference type 'double' to keep the copy or type 'const int &' to prevent copying}}
+
+  for (const Bar x : bar_container) {}
+  // expected-warning@-1 {{loop variable 'x' of type 'const Bar' creates a copy from type 'const Bar'}}
+  // expected-note@-2 {{use reference type 'const Bar &' to prevent copying}}
+}
+
+void test1() {
+  Container<int> A;
+
+  for (const int &x : A) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'int'}}
+  for (const int x : A) {}
+  // No warning, non-reference type indicates copy is made
+  //for (int &x : A) {}
+  // Binding error
+  for (int x : A) {}
+  // No warning, non-reference type indicates copy is made
+
+  for (const double &x : A) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'double'}}
+  for (const double x : A) {}
+  // No warning, non-reference type indicates copy is made
+  //for (double &x : A) {}
+  // Binding error
+  for (double x : A) {}
+  // No warning, non-reference type indicates copy is made
+
+  for (const Bar &x : A) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  for (const Bar x : A) {}
+  // No warning, non-reference type indicates copy is made
+  //for (Bar &x : A) {}
+  // Binding error
+  for (Bar x : A) {}
+  // No warning, non-reference type indicates copy is made
+}
+
+void test2() {
+  Container<int&> B;
+
+  for (const int &x : B) {}
+  // No warning, this reference is not a temporary
+  for (const int x : B) {}
+  // No warning on POD copy
+  for (int &x : B) {}
+  // No warning
+  for (int x : B) {}
+  // No warning
+
+  for (const double &x : B) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'double'{{.*}}'const int &'}}
+  for (const double x : B) {}
+  //for (double &x : B) {}
+  // Binding error
+  for (double x : B) {}
+  // No warning
+
+  for (const Bar &x : B) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note@-2 {{'Bar'}}
+  for (const Bar x : B) {}
+  //for (Bar &x : B) {}
+  // Binding error
+  for (Bar x : B) {}
+  // No warning
+}
+
+void test3() {
+  Container<Bar> C;
+
+  for (const Bar &x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  for (const Bar x : C) {}
+  // No warning, non-reference type indicates copy is made
+  //for (Bar &x : C) {}
+  // Binding error
+  for (Bar x : C) {}
+  // No warning, non-reference type indicates copy is made
+
+  for (const int &x : C) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'int'}}
+  for (const int x : C) {}
+  // No warning, copy made
+  //for (int &x : C) {}
+  // Binding error
+  for (int x : C) {}
+  // No warning, copy made
+}
+
+void test4() {
+  Container<Bar&> D;
+
+  for (const Bar &x : D) {}
+  // No warning, this reference is not a temporary
+  for (const Bar x : D) {}
+  // expected-warning@-1 {{creates a copy}}
+  // expected-note@-2 {{'const Bar &'}}
+  for (Bar &x : D) {}
+  // No warning
+  for (Bar x : D) {}
+  // No warning
+
+  for (const int &x : D) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'int'{{.*}}'const Bar &'}}
+  for (const int x : D) {}
+  // No warning
+  //for (int &x : D) {}
+  // Binding error
+  for (int x : D) {}
+  // No warning
+}
+
+void test5() {
+  Container<Foo> E;
+
+  for (const Bar &x : E) {}
+  // expected-warning@-1 {{always a copy}}
+  // expected-note@-2 {{'Bar'}}
+  for (const Bar x : E) {}
+  // No warning, non-reference type indicates copy is made
+  //for (Bar &x : E) {}
+  // Binding error
+  for (Bar x : E) {}
+  // No warning, non-reference type indicates copy is made
+}
+
+void test6() {
+  Container<Foo&> F;
+
+  for (const Bar &x : F) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'Bar'{{.*}}'const Foo &'}}
+  for (const Bar x : F) {}
+  // No warning.
+  //for (Bar &x : F) {}
+  // Binding error
+  for (Bar x : F) {}
+  // No warning
+}
+
+void test7() {
+  double G[2];
+
+  for (const double &x : G) {}
+  // No warning
+  for (const double x : G) {}
+  // No warning on POD copy
+  for (double &x : G) {}
+  // No warning
+  for (double x : G) {}
+  // No warning
+
+  for (const int &x : G) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'int'{{.*}}'const double &'}}
+  for (const int x : G) {}
+  // No warning
+  //for (int &x : G) {}
+  // Binding error
+  for (int x : G) {}
+  // No warning
+
+  for (const Bar &x : G) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'Bar'{{.*}}'const double &'}}
+  for (const Bar x : G) {}
+  // No warning
+  //for (int &Bar : G) {}
+  // Binding error
+  for (int Bar : G) {}
+  // No warning
+}
+
+void test8() {
+  Foo H[2];
+
+  for (const Foo &x : H) {}
+  // No warning
+  for (const Foo x : H) {}
+  // No warning on POD copy
+  for (Foo &x : H) {}
+  // No warning
+  for (Foo x : H) {}
+  // No warning
+
+  for (const Bar &x : H) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'Bar'{{.*}}'const Foo &'}}
+  for (const Bar x : H) {}
+  // No warning
+  //for (Bar &x: H) {}
+  // Binding error
+  for (Bar x: H) {}
+  // No warning
+}
+
+void test9() {
+  Bar I[2] = {1,2};
+
+  for (const Bar &x : I) {}
+  // No warning
+  for (const Bar x : I) {}
+  // expected-warning@-1 {{creates a copy}}
+  // expected-note@-2 {{'const Bar &'}}
+  for (Bar &x : I) {}
+  // No warning
+  for (Bar x : I) {}
+  // No warning
+
+  for (const int &x : I) {}
+  // expected-warning@-1 {{resulting in a copy}}
+  // expected-note-re@-2 {{'int'{{.*}}'const Bar &'}}
+  for (const int x : I) {}
+  // No warning
+  //for (int &x : I) {}
+  // Binding error
+  for (int x : I) {}
+  // No warning
+}