[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 15 08:32:53 PDT 2017


JonasToth added a comment.

I did read through it now. In general, the code is sound with my understanding of the complexity metric and there is a almost one to one wording to the document, which is nice.
Since we chatted in IRC directly, i would give a short summary to avoid forgetting what we talked about :)

- the enum is specially optimized for size. the bitfield representation does not directly give a benefit, but will force maintainers to think about the enum size
- `Increment; Traverse; Decrement;` is for checking the newly created nesting level -> this was unclear to me, but after clarification pretty easy to understand
- maybe add additional diagnostics on to deep nesting, sidetalk
- class traversal not necessary, since the methods are done directly. Maybe later addition to find complex classes
- some testcases for templatefunctions to demonstrate basic functionality
- one testcase for a standalone class, that is not defined within a function body.



================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:228-234
+    // if   increases nesting level
+    ++CurrentNestingLevel;
+    ShouldContinue = TraverseStmt(Node->getThen());
+    --CurrentNestingLevel;
+
+    if (!ShouldContinue || !Node->getElse())
+      return ShouldContinue;
----------------
line 228-234 are a pattern, existing in all traversals. i think it could be refactored into its own function with a descriptive name. Especially the increment, traverse, decrement isn't obvious immediatly, but when noting its blockyness it is.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:507
+
+  bool TraverseDecl(Decl *Node, bool MainAnalyzedFunction = false) {
+    if (!Node || MainAnalyzedFunction)
----------------
I assume this is the part applicable to class complexity? Maybe a short comment would clarify.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:584
+  for (const auto &Detail : Visitor.CC.Details) {
+    const std::pair<const char *, unsigned short> Reasoning(Detail.Process());
+    diag(Detail.Loc, Reasoning.first, DiagnosticIDs::Note)
----------------
What would you think about the following:
```
const char *IncreaseMessage = nullptr;
unsigned short Increase = 0;
std::tie(IncreaseMessage, Increase) = Reasoning(Detail.Process());
```

Would be applicable elsewhere too. It's more going to the C++17 structured bindings and good potential target for futurue `modernize-` checks ;)
And i think it would be more readable.


================
Comment at: docs/ReleaseNotes.rst:138
+
+  Checks function Cognitive Complexity metric, and flags the functions with
+  Cognitive Complexity exceeding the configured limit.
----------------
How about
'Applies the Cognitive Complexity metric to functions (classes?), flagging those exceeding the configured limit/threshold'


================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:10
+<https://www.sonarsource.com/docs/CognitiveComplexity.pdf>`_ specification
+version 1.2 (19 April 2017), with two notable exceptions:
+   * `preprocessor conditionals` (`#ifdef`, `#if`, `#elif`, `#else`, `#endif`)
----------------
One or two sentences for the general thought would be nice for the quick reader, who doesn't like pdfs.

Noting, that the metric adds additional penalties for nesting and that switch cases are not the same complexity as if/else-if chains would be enough.


================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:11
+version 1.2 (19 April 2017), with two notable exceptions:
+   * `preprocessor conditionals` (`#ifdef`, `#if`, `#elif`, `#else`, `#endif`)
+     are not accounted for. Could be done.
----------------
This should land in a sperate `Limitations` section, similar to other checks.


================
Comment at: test/clang-tidy/readability-function-cognitive-complexity.cpp:23
+// in one go. none of the following should increase cognitive complexity:
+void unittest_false() {
+  {};
----------------
What happens in a codeblock like this:

```
void f() {
  { // nesting is increased, but not complexity, right? There is no occurence of a 'count' event
    throw i;
  }
}
```


================
Comment at: test/clang-tidy/readability-function-cognitive-complexity.cpp:227
+// CHECK-NOTES: :[[@LINE-3]]:14: note: +1{{$}}
+  }
+}
----------------
what happens if you mix the unary operator in?

`if(1 || (1 && !1) || !boolean)`


================
Comment at: test/clang-tidy/readability-function-cognitive-complexity.cpp:481
+// CHECK-NOTES: :[[@LINE-1]]:13: note: +2, including nesting penalty of 1, nesting level increased to 2{{$}}
+// FIXME: would be nice to point at the '?' symbol. does not seem to be possible
+  }
----------------
did you try to get `getTrueExpr`/`getFalseExpr` and point to their respective end/beginning?


Repository:
  rL LLVM

https://reviews.llvm.org/D36836





More information about the cfe-commits mailing list