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

[Richard Trieu]

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:41
@@ +40,3 @@
+
+  for (const int x : int_ref_container) {}
+  // expected-warning at -1 {{loop variable 'x' of type 'const int' creates a copy from type 'int'}}
----------------
David Blaikie wrote:
> Richard Trieu wrote:
> > David Blaikie wrote:
> > > I think this is going to have a pretty high false positive rate... do you have some numbers?
> > What is your idea of a false positive?
> 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?
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.

http://reviews.llvm.org/D4169