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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:30-31
+  bool VisitDecompositionDecl(DecompositionDecl *) {
+    // DecompositionDecl was already visited as VarDecl. Don't count it twice.
+    Info.Variables--;
+    return true;
----------------
aaron.ballman wrote:
> This comment could be clarified. I think what it's trying to express is that the decomposition declaration was already counted as a variable declaration but we do not want to count it as such.
> 
> I think a better way to express this is to change `VisitVarDecl()` to check `isa<DecompositionDecl>()` and not increment in that case, rather than increment in one place and decrement in another. You could also check `isa<ParmVarDecl>()` at the same time and skip subtracting off formal parameters.
> This comment could be clarified. I think what it's trying to express is that the decomposition declaration was already counted as a variable declaration but we do not want to count it as such.

Yep.

> I think a better way to express this is to change VisitVarDecl() to check isa<DecompositionDecl>() and not increment in that case, rather than increment in one place and decrement in another.

I agree.

> You could also check isa<ParmVarDecl>() at the same time and skip subtracting off formal parameters.

That is a behavior-changing change, actually.
But i don't have any strong preferences here, so changed, test added.


================
Comment at: docs/clang-tidy/checks/readability-function-size.rst:42
+
+   Flag functions exceeding this number of variables declared in the body.
+   The default is `-1` (ignore the number of variables).
----------------
aaron.ballman wrote:
> This should clarify that parameters do not count as variables declared in the body.
This gets oh so more interesting with the testcase i added.
I'm not sure whether we actually want to ignore all the `ParmVarDecl`, or keep
```
  unsigned ActualNumberParameters = Func->getNumParams();
  unsigned BodyVariables = FI.Variables - ActualNumberParameters;
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44602