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

[David Blaikie]

Optional feedback left (just restating stuff I already said, perhaps it sounds good to you, but don't feel like you have to agree with me on any of those). Otherwise looks good, commit away!


================
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{{}}
----------------
rtrieu wrote:
> 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.  
Could we just leave a comment in, explaining that this particular combination is invalid? (that way each test is consistent/follows the same formula, but we just explain why this particular intersection isn't relevant/tested)

I'm just a little scared of semi-random note/error-ignorance in a test & how that might hide some other behavior, maybe... not sure. I'm willing to set aside my preference here, though - said my piece & leave it up to you.

================
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}}
+
----------------
rtrieu wrote:
> 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.
I don't think the intent is that clear - it's pretty common to mark references const when you're not planning to mutate them (sometimes out of necessity, because an API only provides a const reference, so you have to follow suit) - but it's pretty rare to put const on local value variables.

I cc'd you on an an internal bug discussing similar behavior in clang-tidy checks for passing by value (I'm not sure if this clang-tidy check is public, the discussion just happened to be internal) which, by the sounds of things, does what I'm suggesting here, and drops const when dropping reference-ness.

I think that's probably the right thing to do.

================
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'}}
----------------
rtrieu wrote:
> 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.
Curious - that seems counter to the usual habit, when writing normal code, to put a comment before the code it applies to. I wouldn't mind breaking in that direction given that there's sufficient variation in the population (though, admittedly, my grepping showed up 173:134 in favor of trailing @- rather than leading @+.. *shrug*)

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

http://reviews.llvm.org/D4169

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