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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: test/clang-tidy/readability-function-size.cpp:207-212
+void variables_8() {
+  int a, b;
+  struct A {
+    A(int c, int d);
+  };
+}
----------------
aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > lebedev.ri wrote:
> > > > aaron.ballman wrote:
> > > > > I think the current behavior here is correct and the previous behavior was incorrect. However, it brings up an interesting question about what to do here:
> > > > > ```
> > > > > void f() {
> > > > >   struct S {
> > > > >     void bar() {
> > > > >       int a, b;
> > > > >     }
> > > > >   };
> > > > > }
> > > > > ```
> > > > > Does `f()` contain zero variables or two? I would contend that it has no variables because S::bar() is a different scope than f(). But I can see a case being made about the complexity of f() being increased by the presence of the local class definition. Perhaps this is a different facet of the test about number of types?
> > > > As previously briefly discussed in IRC, i **strongly** believe that the current behavior is correct, and `readability-function-size`
> > > > should analyze/diagnose the function as a whole, including all sub-classes/sub-functions.
> > > Do you know of any coding standards related to this check that weigh in on this?
> > > 
> > > What do you think about this:
> > > ```
> > > #define SWAP(x, y) ({__typeof__(x) temp = x; x = y; y = x;})
> > > 
> > > void f() {
> > >   int a = 10, b = 12;
> > >   SWAP(a, b);
> > > }
> > > ```
> > > Does f() have two variables or three? Should presence of the `SWAP` macro cause this code to be more complex due to having too many variables?
> > Datapoint: the doc (`docs/clang-tidy/checks/readability-function-size.rst`) actually already states that macros *are* counted.
> > 
> > ```
> > .. option:: StatementThreshold
> > 
> >    Flag functions exceeding this number of statements. This may differ
> >    significantly from the number of lines for macro-heavy code. The default is
> >    `800`.
> > ```
> > ```
> > .. option:: NestingThreshold
> > 
> >     Flag compound statements which create next nesting level after
> >     `NestingThreshold`. This may differ significantly from the expected value
> >     for macro-heavy code. The default is `-1` (ignore the nesting level).
> > ```
> My concerns relate to what's considered a "variable declared in the body" (per the documentation) in relation to function complexity. To me, if the variable is not accessible lexically within the body of the function, it's not adding to the function's complexity *for local variables*. It may certainly be adding other complexity, of course.
> 
> I would have a very hard time explaining to a user that variables they cannot see or change (assuming the macro is in a header file out of their control) contribute to their function's complexity. Similarly, I would have difficulty explaining that variables in an locally declared class member function contribute to the number of variables in the outer function body, but the class data members somehow do not.
> 
> (per the documentation) 

Please note that the word `complexity` is not used in the **documentation**, only `size` is.

There also is the other side of the coin:

```
#define simple_macro_please_ignore \
  the; \
  actual; \
  content; \
  of; \
  the; \
  foo();

// Very simple function, nothing to see.
void foo() {
  simple_macro_please_ignore();
}

#undef simple_macro_please_ignore
```

In other words, if we ignore macros, it would be possible to abuse them to artificially reduce complexity, by hiding it in the macros.
I agree that it's total abuse of macros, but macros are in general not nice, and it would not be good to give such things a pass.


> My concerns relate to what's considered a "variable declared in the body" (per the documentation) in relation to function complexity.

Could you please clarify, at this point, your concerns are only about this new part of the check (variables), or for the entire check?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44602