Extend clang-tidy readability-function-size to allow -1 thresholds and include explicit constructor member initializations count?

[bartlettroscoe]

We are using the clang-tidy checks readability-function-cognitive-complexity and readability-function-size to extract function metrics and produce function metrics data as JSON (by scrapping clang-tidy output) that produces nice metrics like:

  {
    "name": "ArrayTools_Test04",
    "type": "function",
    "location": "packages/intrepid2/unit-test/Shared/ArrayTools/test_04.hpp:46:9",
    "code-line": "int ArrayTools_Test04(const bool verbose) {",
    "cognitive-complexity": 968,
    "nesting-level": 7,
    "num-lines": 3656,
    "num-statements": 5201,
    "num-branches": 422,
    "num-parameters": 1,
    "num-variables": 943
  }

by setting all the thresholds to 0 with the clang-tidy config file:

CheckOptions:
  - key: readability-function-cognitive-complexity.Threshold
    value: '0'
  - key: readability-function-cognitive-complexity.DescribeBasicIncrements
    value: true
  - key: readability-function-cognitive-complexity.IgnoreMacros
    value: true
  - key: readability-function-size.LineThreshold
    value: '0'
  - key: readability-function-size.StatementThreshold
    value: '0'
  - key: readability-function-size.BranchThreshold
    value: '0'
  - key: readability-function-size.ParameterThreshold
    value: '0'
  - key: readability-function-size.NestingThreshold
    value: '0'
  - key: readability-function-size.VariableThreshold
    value: '0'

With this, we can see all functions that have non-zero lines, statements, or parameters. (And after #131040 is addressed, we will be able to see these metrics for constructors and lambda functions as well.)

But even with #131040 addressed, we still will not be able to see functions that have zero lines, zero statements, and zero parameters. What functions have zero lines, zero statements, and zero parameters? => 1) Constructors like:

  SomeClass():
    var1_(<val1>),
    val2_(<val2>),
    val3_(<val3>),
    val4_(<val4>)
    {}

and 2) function overrides for no-ops like:

   int someFunc() override {};

If we could have the readability-function-size checks allow for -1 thresholds, then it would produce output for all functions.

Also, explicit constructor member initializations are code that needs to be maintained, just like variable initialization code inside of a function, so it would be great if the readability-function-size check would report on the number of explicit constructor member initialization counts. This could be reported like:

<source>:<line>:<col>: warning: function 'SomeClass' exceeds recommended size/complexity thresholds [readability-function-size]
  <line> |   SomeClass():
         |   ^
<source>:<line>:<col>: note: 4 member initializers (threshold 0)

Making these changes would have several advantages:

1. By setting the thresholds to -1, we could use readability-function-size to provide a listing of all functions in a C++ code base and see how many functions have zero parameters, zero statements, etc. (Even non-constructor functions with zero parameters and zero statements represent code that has to be maintained.)
2. Constructor variable initializations are real code, just like variable initializations inside of a function, and need to be considered when considering the size of a function implementation.

NOTE: Even though there is no executable code associated with functions with zero statements, there is executable code for constructor member initializers.

NOTE: To expand on our code metrics, we would really love to see clang-tidy add checks for the size of C++ classes like number of data members (public, protected, private, static, etc.), number of member functions (public, protected, private, constructors, virtual, override, etc.), number of typedefs/using declarations (public, protected, private, etc.), etc. Currently, the only check that I can see that provides some visibility into class metrics would be misc-non-private-member-variables-in-classes. (I will create a separate issue for that request.)

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Roscoe A. Bartlett (bartlettroscoe)

We are using the `clang-tidy` checks `readability-function-cognitive-complexity` and `readability-function-size` to extract function metrics and produce function metrics data as JSON (by scrapping `clang-tidy` output) that produces nice metrics like:

  {
    "name": "ArrayTools_Test04",
    "type": "function",
    "location": "packages/intrepid2/unit-test/Shared/ArrayTools/test_04.hpp:46:9",
    "code-line": "int ArrayTools_Test04(const bool verbose) {",
    "cognitive-complexity": 968,
    "nesting-level": 7,
    "num-lines": 3656,
    "num-statements": 5201,
    "num-branches": 422,
    "num-parameters": 1,
    "num-variables": 943
  }

by setting all the thresholds to 0 with the clang-tidy config file:

CheckOptions:
  - key: readability-function-cognitive-complexity.Threshold
    value: '0'
  - key: readability-function-cognitive-complexity.DescribeBasicIncrements
    value: true
  - key: readability-function-cognitive-complexity.IgnoreMacros
    value: true
  - key: readability-function-size.LineThreshold
    value: '0'
  - key: readability-function-size.StatementThreshold
    value: '0'
  - key: readability-function-size.BranchThreshold
    value: '0'
  - key: readability-function-size.ParameterThreshold
    value: '0'
  - key: readability-function-size.NestingThreshold
    value: '0'
  - key: readability-function-size.VariableThreshold
    value: '0'

With this, we can see all functions that have non-zero lines, statements, or parameters. (And after #131040 is addressed, we will be able to see these metrics for constructors and lambda functions as well.)

But even with #131040 addressed, we still will not be able to see functions that have zero lines, zero statements, and zero parameters. What functions have zero lines, zero statements, and zero parameters? => 1) Constructors like:

  SomeClass():
    var1_(<val1>),
    val2_(<val2>),
    val3_(<val3>),
    val4_(<val4>)
    {}

and 2) function overrides for no-ops like:

   int someFunc() override {};

If we could have the readability-function-size checks allow for -1 thresholds, then it would produce output for all functions.

Also, explicit constructor member initializations are code that needs to be maintained, just like variable initialization code inside of a function, so it would be great if the readability-function-size check would report on the number of explicit constructor member initialization counts. This could be reported like:

<source>:<line>:<col>: warning: function 'SomeClass' exceeds recommended size/complexity thresholds [readability-function-size]
  <line> |   SomeClass():
         |   ^
<source>:<line>:<col>: note: 4 member initializers (threshold 0)

Making these changes would have several advantages:

1. By setting the thresholds to -1, we could use readability-function-size to provide a listing of all functions in a C++ code base and see how many functions have zero parameters, zero statements, etc. (Even non-constructor functions with zero parameters and zero statements represent code that has to be maintained.)
2. Constructor variable initializations are real code, just like variable initializations inside of a function, and need to be considered when considering the size of a function implementation.

NOTE: Even though there is no executable code associated with functions with zero statements, there is executable code for constructor member initializers.

NOTE: To expand on our code metrics, we would really love to see clang-tidy add checks for the size of C++ classes like number of data members (public, protected, private, static, etc.), number of member functions (public, protected, private, constructors, virtual, override, etc.), number of typedefs/using declarations (public, protected, private, etc.), etc. Currently, the only check that I can see that provides some visibility into class metrics would be misc-non-private-member-variables-in-classes. (I will create a separate issue for that request.)

[PiotrZSL]

"By setting the thresholds to -1, we could use readability-function-size to provide a listing of all functions in a C++ code base and see how many functions have zero parameters, zero statements, etc. (Even non-constructor functions with zero parameters and zero statements represent code that has to be maintained.)"

Ok but thats not an purpose of the check.

As for constructor initializes, that probably could be counted to an Statements and Branches.

[bartlettroscoe]

"By setting the thresholds to -1, we could use readability-function-size to provide a listing of all functions in a C++ code base and see how many functions have zero parameters, zero statements, etc. (Even non-constructor functions with zero parameters and zero statements represent code that has to be maintained.)"

Ok but thats not an purpose of the check.

Understood. But having a tool that can give reliable metrics for all C++ code is pretty darned useful.

As for constructor initializes, that probably could be counted to an Statements and Branches.

That would be great :-)

We can live without metrics for functions with no parameters and no branches.

[vbvictor]

Actually, we can not use "-1" value to warn on "everything" because "-1" already means no threshold. With such change we may break existing users.
See

llvm-project/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.h

Line 20 in e57cd10

/// * `LineThreshold` - flag functions exceeding this number of lines. The

llvm-project/clang-tools-extra/clang-tidy/ClangTidyCheck.h

Line 280 in e57cd10

/// and equals ``none``, ``null``, ``-1`` or empty, return ``std::nullopt``.

[bartlettroscoe]

Actually, we can not use "-1" value to warn on "everything" because "-1" already means no threshold. With such change we may break existing users.

@vbvictor, understood. I know that the idea of using clang-tidy checks as a metrics tool for C++ is not what clang-tidy was designed to do, but these checks are so close to being able to perform that role. (And if we don't care about getting metrics for functions with no parameters or statements, then once we address #131040, we will have what we need.)

I will open-source the clang-tidy metrics tools that I have created to demonstrate the value of these metrics and then we can see about generally extending and adding new clang-tidy checks to flesh out the set of software metrics that can be produced.

[bartlettroscoe]

An alternative to allowing -1 thresholds would be to add a new option called something like ReportAllFunctions that when set to true would produce output for all functions, even those that had zero params and statements. Other checks have Boolean options so there is precedent for this.

NOTE: Even the documentation for readability-function-size admits these are "metrics":

Checks for large functions based on various metrics.

[vbvictor]

@bartlettroscoe, as for readability-function-size check, I hope to make improvements in the near future (if no one does first).
lambdas currently not matched by design, but I think an option can be made for that.

llvm-project/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp

Line 151 in e0223fa

// FIXME: include them instead? Top-level lambdas are currently never counted.

I will open-source the clang-tidy metrics tools that I have created to demonstrate the value of these metrics and then we can see about generally extending and adding new clang-tidy checks to flesh out the set of software metrics that can be produced.

Sounds interesting, I'm looking forwart to try it.

[bartlettroscoe]

ASIDE: As I think about this, it the future, it would be great to have a check like this print the attributes for functions like static, member (and the parent class) (or free), namespace (and the parent namespace), inline, extern, const (or non-const), constexpr, consteval, noexpect, mutable, volitile, various modifiers (e.g. virtual, override, final), and various standard attributes (e.g.[[noreturn]], [[deprecated]], etc.). That would just be a list of attributes like:

<location>: note: function attributes: <attr1>, <attr2>, ...

That provides more context about the type of function (and would be very useful for metrics analysis).

Also, another type of legitimate numeric function metric would be num_template_params (integer >= 0) (TemplateParametersThreshold) reported like:

<location>: note: <num-template-parameters> template parameters (threshold <num-template-parameters-threashold>)

But asking for these things would be a different issue 😊

[vbvictor]

template-parameter-threshold may be put under readability-function-size check, but function-attributes may be debatable since it doesn't fall into "linting" directly.
For function-attributes I can suggest writing an out-of-tree check for your needs. If you are familiar with LLVM and AST-matchers this may not be hard at all.

[bartlettroscoe]

lambdas currently not matched by design, but I think an option can be made for that.

That would be great! Thanks for considering this! I will respond more over in #131040. (See #131040 (comment))