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