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

[David Blaikie]

================
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'}}
----------------
Richard Trieu wrote:
> 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.
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.

http://reviews.llvm.org/D4169