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

[David Blaikie]

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

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

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

================
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{{}}
----------------
Is it much hassle to just have the code work in such a way that it doesn't have errors?

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

================
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}}
+
----------------
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)

================
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'}}
----------------
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?

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

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

http://reviews.llvm.org/D4169

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