[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D44602#1041153, @JonasToth wrote:

> Looks fine to me, but Aaron or someone else must accept :)


Thanks for review!

In https://reviews.llvm.org/D44602#1041153, @JonasToth wrote:

> Just out of curiosity: The decomposition visit is for structured binding and the binding visit for range based for? Are there other places where binding occurs, for example in some template stuff? 
>  Just asking if this might hit me in a other check :D


The comment i added is actually wrong.
The range-based for just works anyway.
The `VisitBindingDecl()` is needed to get all the 'variables' declared in structured binding.
But as you can see in https://godbolt.org/g/be6Juf, the `BindingDecl`'s are wrapped into `DecompositionDecl`.
And `DecompositionDecl` is actually inherited from `VarDecl`, https://github.com/llvm-mirror/clang/blob/b031fdc9b7dbd9c7f942d8060a4f00d63f3c9af2/include/clang/AST/DeclCXX.h#L3824-L3825,
so we count the `DecompositionDecl` in `VisitVarDecl()`, thus we need a `VisitDecompositionDecl()` that subtracts it.

This may be the wrong/incomplete solution.



================
Comment at: docs/ReleaseNotes.rst:125
+
+  Flags functions exceeding this number of variables declared in the body.
+
----------------
JonasToth wrote:
> I would rephrase this a little to:
> 
> ```
>  Flags function bodies exceeding this number of declared variables.
> ```
I agree that it looks gibberish. Reworded a little. Probably still not ideal.


================
Comment at: test/clang-tidy/readability-function-size.cpp:118
   }
+  // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 6 variables (threshold 1)
 }
----------------
JonasToth wrote:
> Why is this check here and not on line 100 like the other conditions?
It can not be there, the `note: nesting level <> starts here (threshold 2)` are outputted before `note: <> variables (threshold 1)`, and filecheck respects the order of `// CHECK-MESSAGES:`


================
Comment at: test/clang-tidy/readability-function-size.cpp:153
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+void variables_1(int i) {
+  int j;
----------------
JonasToth wrote:
> Could you please add a test for function having more parameters then allowed variables, but no variables?
It's already checked in `foo7()`, but sure.


================
Comment at: test/clang-tidy/readability-function-size.cpp:180
+// CHECK-MESSAGES: :[[@LINE-6]]:6: note: 2 variables (threshold 1)
+void variables_5() {
+  for (int i;;)
----------------
JonasToth wrote:
> This testfile is not C++17 right now, but do you know how structured bindings are handled? Maybe its worth adding another file for C++17 testing it.
> 
> Could you add a little test for range based for loops?
> This testfile is not C++17 right now, but do you know how structured bindings are handled? Maybe its worth adding another file for C++17 testing it.

Nice catch!

> Could you add a little test for range based for loops?

Added. I expected them to not work (after a quick look at AST), but apparently they just work..


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44602