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

[Richard Trieu]

================
Comment at: lib/Sema/SemaStmt.cpp:2412
@@ +2411,3 @@
+      E = MTE->GetTemporaryExpr();
+    } else {
+      llvm_unreachable("Unexpected expression encountered.");
----------------
dblaikie wrote:
> Avoid branch-to-unreachable, just uncoditionally cast<MaterializeTemporaryExpr> in the previous block (as an else, not an else-if)
Done.

================
Comment at: lib/Sema/SemaStmt.cpp:2427
@@ +2426,3 @@
+  } else {
+    llvm_unreachable("Unexpected expression encountered.");
+  }
----------------
dblaikie wrote:
> 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)
Done.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:13
@@ +12,3 @@
+template <typename T>
+class Container {
+  typedef Iterator<T&> I;
----------------
dblaikie wrote:
> 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.
Done.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:17
@@ +16,3 @@
+public:
+  // These notes are from errors, which this test doesn't care much about.
+  I begin();  // expected-note 4{{}}
----------------
dblaikie wrote:
> Is it much hassle to just have the code work in such a way that it doesn't have errors?
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.  

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:23
@@ +22,3 @@
+template <typename T>
+class ContainerNonReference {
+  typedef Iterator<T> I;
----------------
dblaikie wrote:
> 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.
Done.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:87
@@ +86,3 @@
+  // expected-warning at -1 {{loop variable 'x' has type 'const double &' but is initialized with type 'int' resulting in a copy}}
+  // expected-note at -2 {{use non-reference type 'const double' to keep the copy or type 'const int &' to prevent copying}}
+
----------------
dblaikie wrote:
> 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)
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.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:98
@@ +97,3 @@
+  for (const int &x : A) {}
+  // expected-warning at -1 {{always a copy}}
+  // expected-note at -2 {{'const int'}}
----------------
dblaikie wrote:
> 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?
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.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:180
@@ +179,3 @@
+  for (int x : C) {}
+  // Now warning, copy made
+}
----------------
dblaikie wrote:
> typo, 'no' rather than 'now'?
Fixed.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:292
@@ +291,3 @@
+void test9() {
+  Bar I[2] = {1,2};
+
----------------
dblaikie wrote:
> 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)
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.

http://reviews.llvm.org/D4169

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/