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

[Alp Toker]

On 22/06/2014 18:37, David Blaikie wrote:
> ================
> 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.

Yes, parsing and semantic analysis shouldn't be concerned with 
diagnostic levels other than opportunistically checking if the 
diagnostic is definitely ignored.

This separation of concerns means it'll become possible to support 
interactive post-compile diagnostic mapping which is a great timesaver 
for IDE and analysis tool workflows.

(In the meantime, it also lets us validate that enabling warning flags 
doesn't affect code generation or AST output -- unfortunately this does 
happen in places.)

Alp.


>
> ================
> 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
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-- 
http://www.nuanti.com
the browser experts