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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:367-368
+  bool TraverseStmt(Stmt *Node) {
+    if (!Node)
+      return Base::TraverseStmt(Node);
+
----------------
aaron.ballman wrote:
> If there's not a node, do we really need to traverse it?
This is consistent across this whole `FunctionASTVisitor`, and is consistent with other `RecursiveASTVisitor<>`s,
so i'll leave this as-is. If this is wrong, other places should be changed too.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:381
+    // FIXME: "each method in a recursion cycle" Increment is not implemented.
+    case Stmt::ConditionalOperatorClass:
+    case Stmt::SwitchStmtClass:
----------------
aaron.ballman wrote:
> `BinaryConditionalOperatorClass` as well (for all the places we're dealing with `ConditionalOperatorClass`)?
No, `BinaryConditionalOperator` is explicitly exempt.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:388
+    case Stmt::CXXCatchStmtClass:
+    case Stmt::GotoStmtClass:
+      Reasons |= CognitiveComplexity::Criteria::Increment;
----------------
aaron.ballman wrote:
> Should `IndirectGotoStmtClass` be handled the same way?
Right, it should be.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:410
+    case Stmt::CXXCatchStmtClass:
+    case Stmt::LambdaExprClass:
+      Reasons |= CognitiveComplexity::Criteria::IncrementNesting;
----------------
aaron.ballman wrote:
> `BlockExprClass` as well?
> 
> Should GNU statement expressions also be treated like a lambda or block?
> BlockExprClass as well?

Thank you for the example, that seems obvious & straight-forward to support.
As discussed, we should handle decls thought, not exprs.

> Should GNU statement expressions also be treated like a lambda or block?

Hm, i guess it should be.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:428
+    case Stmt::DoStmtClass:
+    case Stmt::CXXCatchStmtClass:
+      Reasons |= CognitiveComplexity::Criteria::PenalizeNesting;
----------------
aaron.ballman wrote:
> `SEHExceptStmtClass` as well?
I'm very much unfamiliar with that extension.
I would prefer to leave it as-is for now.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:473
+    case Decl::CXXConstructor:
+    case Decl::CXXDestructor:
+      break;
----------------
aaron.ballman wrote:
> What about other special member functions like overloaded operators and whatnot? How about block declarations?
> 
> Namespaces is another one I was curious about but suspect may not want to be handled here.
> What about other special member functions like overloaded operators and whatnot?

Those are `CXXMethodDecl`, but i'll add a test.

> How about block declarations?

Right, we should handle them, not blockstatements.

> Namespaces is another one I was curious about but suspect may not want to be handled here.

To the best of my knowledge, those can only appear at the global scope.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36836/new/

https://reviews.llvm.org/D36836