[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:2361
@@ +2360,3 @@
+                                           const CXXForRangeStmt *ForStmt) {
+  if (SemaRef.Diags.getDiagnosticLevel(
+          diag::warn_for_range_const_reference_copy, ForStmt->getLocStart()) ==
----------------
David Blaikie wrote:
> 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.
Phabricator doesn't show it, but the revision this patch was based on preceded Alp's patch, so using the isIgnored wasn't an option at the time.  I have rebased to a later revision and now use it.

================
Comment at: lib/Sema/SemaStmt.cpp:2373
@@ +2372,3 @@
+
+  const VarDecl *VD = ForStmt->getLoopVariable();
+  if (!VD)
----------------
David Blaikie wrote:
> What are the cases where a for loop has no loop variable? (or the loop variable has no init, down on line 2382)
There is not guarantee that this is a well-formed for loop.  It is possible that ill-formed code created a less that perfect AST, which we should protect against.

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

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

================
Comment at: lib/Sema/SemaStmt.cpp:2452
@@ +2451,3 @@
+    if (const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(InitExpr)) {
+      if (CE->getConstructor()->isCopyConstructor())
+        MakeCopy = true;
----------------
David Blaikie wrote:
> Not sure if it'd be more readable, but you could just assign:
> 
> MakeCopy = CE->getConstructor()->isCopyConstructor();
Used an early return in the separate function instead of the MakeCopy variable.

================
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;
----------------
David Blaikie wrote:
> Same assignment possible here
Same as above.

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

================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:14
@@ +13,3 @@
+class Container {
+  typedef Iterator<return_type> I;
+
----------------
David Blaikie wrote:
> 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)
Using two separate containers now.  Container uses reference returning iterators.  ContainerNonReference uses non-reference returning iterators.

================
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:
> 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?

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

================
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;
----------------
David Blaikie wrote:
> any reason for the typedef here, but not in test0 above?
Removed typedef.  Relevant information moved to test comments 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}}
----------------
David Blaikie wrote:
> 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?
test0 checks the entire warning and note messages while the remainder of the tests focuses on thoroughly testings the different combinations of types.

Typedefs have been removed as nothing tests them.

================
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
----------------
David Blaikie wrote:
> I worry about people creating expensive copies this way, but maybe that'll need to be a clang-tidy check rather than a warning.
Since there are only two forms of const loop variables, this one must be the one accepted for copies.  Otherwise, we would have to suggest something like:

  for (const int x : A) {
    const Bar y(x);
  }

in order to silence the warning.

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

http://reviews.llvm.org/D4169