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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
