[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:2361
@@ +2360,3 @@
+                                           const CXXForRangeStmt *ForStmt) {
+  if (SemaRef.Diags.getDiagnosticLevel(
+          diag::warn_for_range_const_reference_copy, ForStmt->getLocStart()) ==
----------------
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.

================
Comment at: lib/Sema/SemaStmt.cpp:2373
@@ +2372,3 @@
+
+  const VarDecl *VD = ForStmt->getLoopVariable();
+  if (!VD)
----------------
What are the cases where a for loop has no loop variable? (or the loop variable has no init, down on line 2382)

================
Comment at: lib/Sema/SemaStmt.cpp:2386
@@ +2385,3 @@
+
+  if (VariableType->isReferenceType()) {
+    // Foo foos[5];
----------------
Might be worth splitting out these cases as separate functions, maybe?

================
Comment at: lib/Sema/SemaStmt.cpp:2449
@@ +2448,3 @@
+    // Foo foos[5];
+    // for (const Bar bar : foos);
+    bool MakeCopy = false;
----------------
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?

================
Comment at: lib/Sema/SemaStmt.cpp:2452
@@ +2451,3 @@
+    if (const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(InitExpr)) {
+      if (CE->getConstructor()->isCopyConstructor())
+        MakeCopy = true;
----------------
Not sure if it'd be more readable, but you could just assign:

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

================
Comment at: lib/Sema/SemaStmt.cpp:2455
@@ +2454,3 @@
+    } else if (const CastExpr *CE = dyn_cast<CastExpr>(InitExpr)) {
+      if (CE->getCastKind() == CK_LValueToRValue)
+        MakeCopy = true;
----------------
Same assignment possible here

================
Comment at: lib/Sema/SemaStmt.cpp:2467
@@ +2466,3 @@
+    }
+    return;
+  }
----------------
return here seems a bit unnecessary (and/or as above, this whole case might be nice as a separate function for readability)

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

================
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'}}
----------------
I think this is going to have a pretty high false positive rate... do you have some numbers?

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:46
@@ +45,3 @@
+
+void test1() {
+  typedef Container<int> non_reference_iterator_container;
----------------
If these test functions provide logical grouping, perhaps they could be named based on that grouping, or have a comment describing it?

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:47
@@ +46,3 @@
+void test1() {
+  typedef Container<int> non_reference_iterator_container;
+  non_reference_iterator_container A;
----------------
any reason for the typedef here, but not in test0 above?

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

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:73
@@ +72,3 @@
+  // expected-note at -2 {{'const Bar'}}
+  for (const Bar x : A) {}
+  // No warning, non-reference type indicates copy is made
----------------
I worry about people creating expensive copies this way, but maybe that'll need to be a clang-tidy check rather than a warning.

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:83
@@ +82,3 @@
+  typedef Container<int&> reference_iterator_container;
+  reference_iterator_container B;
+
----------------
Same questions as above

http://reviews.llvm.org/D4169