Adds -Wrange-loop-analysis to -Wall

[Mordante]

Summary

This makes the range loop warnings part of -Wall.

This 'fixes' https://bugs.llvm.org/show_bug.cgi?id=32823

[xbolva00]

Needs a test

[Mordante]

Adds a test as requested by @xbolva00 .

[aaron.ballman]

Does this analysis require CFG support or have a high false positive rate? We typically keep those out of -Wall because of performance concerns, so I am wondering if the diagnostic was kept out of -Wall for a reason.

[xbolva00]

Does this analysis require CFG support

https://reviews.llvm.org/D69292 also requires CFG support.

Generally, is CFG support ok for -Wall if the warning has few false positives?

I am wondering if the diagnostic was kept out of -Wall for a reason.

Yes, it would be good to find why this was disabled (never enabled?).

@Mordante can you compile LLVM itself with this patch?

[aaron.ballman]

In D68912#1723691, @xbolva00 wrote:

Does this analysis require CFG support

https://reviews.llvm.org/D69292 also requires CFG support.

Yes, it does.

Generally, is CFG support ok for -Wall if the warning has few false positives?

I think it also depends on the performance of the diagnostic.

I am wondering if the diagnostic was kept out of -Wall for a reason.

Yes, it would be good to find why this was disabled (never enabled?).

@Mordante can you compile LLVM itself with this patch?

I'd be curious to know how the timing changes between compilations as well; does this regress performance considerably?

[Mordante]

In D68912#1723722, @aaron.ballman wrote:

In D68912#1723691, @xbolva00 wrote:

Does this analysis require CFG support

https://reviews.llvm.org/D69292 also requires CFG support.

Yes, it does.

Generally, is CFG support ok for -Wall if the warning has few false positives?

I think it also depends on the performance of the diagnostic.

I am wondering if the diagnostic was kept out of -Wall for a reason.

Yes, it would be good to find why this was disabled (never enabled?).

I don't know, @rtrieu originally added the warnings, maybe he can answer that question.

@Mordante can you compile LLVM itself with this patch?

I compiled LLVM and Clang and it adds 145 unique warnings. I wouldn't mind to fix those and the other subprojects, so I can test whether the tests have false positives.
Am I correct to assume these patches all need to be reviewed?

I'd be curious to know how the timing changes between compilations as well; does this regress performance considerably?

I did a partial build of LLVM the timings are quite similar:

Before:
real 7m44.918s
user 14m41.052s
sys 0m16.560s

After:
real 7m52.934s
user 14m39.968s
sys 0m16.932s

[xbolva00]

I compiled LLVM and Clang and it adds 145 unique warnings. I wouldn't mind to fix those and the other subprojects, so I can test whether the tests have false positives.
Am I correct to assume these patches all need to be reviewed?

Can you upload the list with new warnings somewhere? How many false positives?

I dont think you have to fix them as a prerequisite - buildbots do not use -Werror.

[Mordante]

I haven't reviewed the list [1] properly so I don't know the number of false positives yet.

[1] http://paste.debian.net/1111806/

[aaron.ballman]

I spot-checked the list and definitely saw some true positives in there. I did not exhaustively check the list, however. The timing numbers look reasonable enough as well. Unless someone finds an issue with the fp rate that I've missed, I think this is reasonable.

[Mordante]

Thanks for the review. I also looked at some of the new warnings and I'm preparing patches to fix them.

[xbolva00]

Formally unblocking this

[Mordante]

In D68912#1737559, @xbolva00 wrote:

Formally unblocking this

Thanks. I'll first want to reduce the number of warnings before committing this patch.

[xbolva00]

I think you can commit it, you already fixed some warnings.

[xbolva00]

In D68912#1754660, @xbolva00 wrote:

I think you can commit it, you already fixed some warnings.

Oh, I hit this (-Werror bots) hard :D Some bots use -Werror. So when build is warning-free, then please commit this patch.

[Mordante]

In D68912#1757850, @xbolva00 wrote:

In D68912#1754660, @xbolva00 wrote:

I think you can commit it, you already fixed some warnings.

Oh, I hit this (-Werror bots) hard :D Some bots use -Werror. So when build is warning-free, then please commit this patch.

I will, a bit busy at the moment but will look at fixing the warnings before committing.

[xbolva00]

All is fixed now?

[Mordante]

In D68912#1794138, @xbolva00 wrote:

All is fixed now?

Not yet :-(
I ran into a false positive with rvalue references, will post a code fix for it and then I'll look into posting more error fixes.

[denik]

We are hitting the warning in template code with bool and loop like this: "for (const auto& element : value)" where element is bool from template. But not always.
When it is bool the warning triggers:
error: loop variable 'element' is always a copy because the range of type 'const std::vector<bool, allocator<bool> >' does not return a reference [-Werror,-Wrange-loop-analysis]

However, the template type can also use non-bools and if I remove the reference I hit alternative case where the warning complains about copy operation of reference types:
error: loop variable 'element' of type 'const std::1::basic_string<char>' creates a copy from type 'const std::1::basic_string<char>' [-Werror,-Wrange-loop-analysis]
for (const auto element : value):
use reference type 'const std::1::tuple<unsigned int, bool, std::1::vector<unsigned char, std::__1::allocator<unsigned char> > > &' to prevent copying

So there is no way to satisfy the warning other than disabling it.

[Mordante]

Thanks for your report. I think it's similar to https://bugs.llvm.org/show_bug.cgi?id=44556. I'll look for a fix.

[Mordante]

I proposed D73007 as fix.

clang/include/clang/Basic/DiagnosticGroups.td

def Extra : DiagGroup<"extra", [
     NullPointerArithmetic,
     EmptyInitStatement
   ]>;
 
 def Most : DiagGroup<"most", [
     CharSubscript,
     Comment,
     DeleteNonVirtualDtor,
-    ForLoopAnalysis,
     Format,
     Implicit,
     InfiniteRecursion,
     IntInBoolContext,
+    LoopAnalysis,
     MismatchedTags,
     MissingBraces,
     Move,
     MultiChar,
     Reorder,
     ReturnType,
     SelfAssignment,
     SelfMove,

clang/test/Misc/warning-wall.c

 RUN: diagtool tree -Wall > %t 2>&1
 RUN: FileCheck --input-file=%t %s
 
      CHECK:-Wall
 CHECK-NEXT:  -Wmost
 CHECK-NEXT:    -Wchar-subscripts
 CHECK-NEXT:    -Wcomment
 CHECK-NEXT:    -Wdelete-non-virtual-dtor
 CHECK-NEXT:      -Wdelete-non-abstract-non-virtual-dtor
 CHECK-NEXT:      -Wdelete-abstract-non-virtual-dtor
-CHECK-NEXT:    -Wfor-loop-analysis
 CHECK-NEXT:    -Wformat
 CHECK-NEXT:      -Wformat-extra-args
 CHECK-NEXT:      -Wformat-zero-length
 CHECK-NEXT:      -Wnonnull
 CHECK-NEXT:      -Wformat-security
 CHECK-NEXT:      -Wformat-y2k
 CHECK-NEXT:      -Wformat-invalid-specifier
 CHECK-NEXT:    -Wimplicit
 CHECK-NEXT:      -Wimplicit-function-declaration
 CHECK-NEXT:      -Wimplicit-int
 CHECK-NEXT:    -Winfinite-recursion
 CHECK-NEXT:    -Wint-in-bool-context
+CHECK-NEXT:    -Wloop-analysis
+CHECK-NEXT:      -Wfor-loop-analysis
+CHECK-NEXT:      -Wrange-loop-analysis
 CHECK-NEXT:    -Wmismatched-tags
 CHECK-NEXT:    -Wmissing-braces
 CHECK-NEXT:    -Wmove
 CHECK-NEXT:      -Wpessimizing-move
 CHECK-NEXT:      -Wredundant-move
 CHECK-NEXT:      -Wreturn-std-move
 CHECK-NEXT:      -Wself-move
 CHECK-NEXT:    -Wmultichar

clang/test/SemaCXX/warn-range-loop-analysis.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wall -Wno-unused -verify %s
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -verify %s
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wrange-loop-analysis -verify %s
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 template <typename return_type>
 struct Iterator {
   return_type operator*();
   Iterator operator++();