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

Sat Oct 21 06:30:28 PDT 2017

aaron.ballman added inline comments.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:77
+  const std::array<const StringRef, 4> Msgs = {{
+      // FIXME: these messages somehow trigger an assertion:
+      // Fix conflicts with existing fix! The new replacement overlaps with an
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > Are you intending to fix this -- that sounds rather serious?
> I do intend to fix it, as soon as i'm aware of how to fix this.
> Specifically, https://reviews.llvm.org/D36836#889350
> ```
> Question:
> Is it expected that clang-tidy somehow parses the DiagnosticIDs::Note's as FixIt's, and thus fails with the following assert:
> ...
> ```
> I.e. what is the proper way to fix this, should i change the message, or change the code not to parse the message as FixIt?
This is one for @alexfh to answer, I think. My gut says that the code should not be parsing notes as fixits (as a separate commit).

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:99
+  /// details nessesary
+  struct Detail final {
+    const SourceLocation Loc;     /// What caused the increment?
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > I don't think the `final` adds value here.
> I don't think it hurts, either?
> I *personally* prefer to specify it always, and remove if subclassing is needed.
> But if you insist i can certainly drop it
That's not the typical coding style for the project, so I would prefer to leave it off. It's reasonable to use when there are vtables involved, however.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102
+    const unsigned short Nesting; /// How deeply nested is Loc located?
+    const Criteria C : 3;         /// The criteria of the increment
+
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > Why is this turned into a bit-field? If that is important, it should be of type `unsigned` to prevent differences between compilers (MSVC treats bit-fields differently than GCC and Clang).
> The comment right after this member variable should have explained it:
> ```
> /// Criteria C is a bitfield. Even though Criteria is an unsigned char; and
> /// only using 3 bits will still result in padding, the fact that it is a
> /// bitfield is a reminder that it is important to min(sizeof(Detail))
> ```
> It is already `unsigned`:
> ```
> enum Criteria : unsigned char {
> ```
No, it's underlying type is `unsigned char`, but the type is `Criteria`. I meant the field itself needs to be `unsigned`. However, since that means you have to do more type casting, I think making the type `uint8_t` instead of a bit-field would be an improvement.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:118
+
+      unsigned MsgId;           /// The id of the message to output
+      unsigned short Increment; /// How much of an increment?
----------------
aaron.ballman wrote:
> We use `//` instead of `///` unless we're specifically documenting something for doxygen.
This change is marked as done but wasn't applied consistently. For instance, see lines 130-131 where it switches between uses. I would use `//` consistently everywhere unless documenting a public interface in a header.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:111
+  };
+  static_assert(sizeof(Detail) <= 8, "it's best to keep the size minimal");
+
----------------
This `static_assert` should come with some further comments explaining that size being small is good, but not critical for correctness.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:212
+  bool TraverseStmtWithIncreasedNestingLevel(Stmt *Node) {
+    bool ShouldContinue;
+    ++CurrentNestingLevel;
----------------
Declare with its initialization below.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:220
+  bool TraverseDeclWithIncreasedNestingLevel(Decl *Node) {
+    bool ShouldContinue;
+    ++CurrentNestingLevel;
----------------
Declare with its initialization below.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:250
+
+    /// If this IfStmt is *NOT* "else if", then only the body (i.e. "Then" and
+    /// "Else" are traversed with increased Nesting level.
----------------
Missing a closing paren in the comment.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:275-276
+
+    if (isa<IfStmt>(Node->getElse()))
+      return TraverseIfStmt(dyn_cast<IfStmt>(Node->getElse()), true);
+
----------------
Would be better to just get the AST node directly instead of `isa<>()` and `dyn_cast<>()`.

`if (const auto *E = dyn_cast<IfStmt>(Node->getElse()))`

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:317
+                                                                               \
+    /** We might encounter function call, which starts a new sequence, thus    \
+     ** we need to save the current previous binary operator. */               \
----------------
encounter function call -> encounter a function call

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:325
+    const bool ShouldContinue = Base::TraverseBin##CLASS(Op);                  \
+    /** And restore the previous binary operator, which might be nonexistant.  \
+     */                                                                        \
----------------
typo: nonexistent

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:332
+
+  /// In a sequence of binary logical operators, if new operator is different
+  /// from the previous-one, then the cognitive complexity is increased.
----------------
if new operator -> if the new operator

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:333
+  /// In a sequence of binary logical operators, if new operator is different
+  /// from the previous-one, then the cognitive complexity is increased.
+  TraverseBinOp(LAnd);
----------------
previous-one -> previous one

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:338
+  /// It would make sense for the function call to start the new binary
+  /// operator sequence, thus let's make sure that it creates new stack frame.
+  bool TraverseCallExpr(CallExpr *Node) {
----------------
creates new stack -> creates a new stack

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:347
+    BinaryOperatorsStack.emplace();
+    const bool ShouldContinue = Base::TraverseCallExpr(Node);
+    /// And remove the top frame.
----------------
Can drop the `const`.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:370
+    /// if, else if, else  are handled in TraverseIfStmt(),
+    /// FIXME: each method in a recursion cycle.
+    case Stmt::ConditionalOperatorClass:
----------------
This FIXME doesn't describe what needs to be fixed very well, you might want to expound (or fix it).

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:384
+      /// but they are not supported in C++ or C.
+      /// regular break/continue do not increase cognitive complexity.
+      break;
----------------
regular -> Regular

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:391
+    switch (Node->getStmtClass()) {
+    /// if, else if, else  are handled in TraverseIfStmt(),
+    /// nested methods and such are handled in TraverseDecl.
----------------
else  are -> else are (remove extra space)

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:392
+    /// if, else if, else  are handled in TraverseIfStmt(),
+    /// nested methods and such are handled in TraverseDecl.
+    case Stmt::ConditionalOperatorClass:
----------------
nested -> Nested

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:411
+    switch (Node->getStmtClass()) {
+    /// if, else if, else  are handled in TraverseIfStmt().
+    case Stmt::ConditionalOperatorClass:
----------------
extra space between else and are

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:433
+
+    /// if we have found any reasons, let's account it.
+    if (Reasons & CognitiveComplexity::Criteria::All)
----------------
if -> If

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:444
+
+  /// The second non-standard parameter MainAnalyzedFunction is needed to
+  /// differentiate between the cases where TraverseDecl() is the entry point
----------------
The second non-standard parameter -> The parameter

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:448
+  /// called from the FunctionASTVisitor itself.
+  /// Explaination: if we get a function definition (e.g. constructor,
+  /// destructor, method), the Cognitive Complexity specification states that
----------------
Explaination -> Explanation

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:495
+  Finder->addMatcher(
+      functionDecl(unless(anyOf(isInstantiated(), isImplicit()))).bind("func"),
+      this);
----------------
You only need to match function *definitions* (not declarations), correct? It might be useful to specify that in the matcher so that it matches less code.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:513-514
+
+  // Now, output all the basic increments, of which this Cognitive Complexity
+  // consists of
+  for (const auto &Detail : Visitor.CC.Details) {
----------------
I would reword this to: Output all the basic increments of complexity.

================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:520
+    assert(MsgId < Msgs.size() && "MsgId should always be valid");
+    // Increase on the other hand, can be 0.
+
----------------
Increase on the -> Increase, on the

================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:8
+
+The metric is implemented as per `COGNITIVE COMPLEXITY by SonarSource
+<https://www.sonarsource.com/docs/CognitiveComplexity.pdf>`_ specification
----------------
as per -> as per the

================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:85
+
+And this is where the previous basic building block, `Nesting level`_, matters.
+The following structures increase the function's Cognitive Complexity metric by
----------------
And this -> This

================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:141
+   * `preprocessor conditionals` (``#ifdef``, ``#if``, ``#elif``, ``#else``,
+     ``#endif``) are not accounted for. Could be done.
+   * `each method in a recursion cycle` is not accounted for. It can't be fully
----------------
I would drop `Could be done.`

Repository:
  rL LLVM

https://reviews.llvm.org/D36836