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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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);
+
----------------
lebedev.ri wrote:
> 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.
Fine by me, thanks.


================
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:
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > `BinaryConditionalOperatorClass` as well (for all the places we're dealing with `ConditionalOperatorClass`)?
> No, `BinaryConditionalOperator` is explicitly exempt.
I didn't see anything in the paper that talked about this (it's a GNU extension). I feel like anywhere we handle a ternary conditional we should also handle a binary conditional, e.g.,
```
foo ? foo : bar; // ternary form
foo ? : bar; // binary form of the same thing
```
but maybe I'm misunderstanding something.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:428
+    case Stmt::DoStmtClass:
+    case Stmt::CXXCatchStmtClass:
+      Reasons |= CognitiveComplexity::Criteria::PenalizeNesting;
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > `SEHExceptStmtClass` as well?
> I'm very much unfamiliar with that extension.
> I would prefer to leave it as-is for now.
I'm fine handling it in a follow-up. I suspect there may be other esoteric AST nodes we want to add, I just tried to hit the ones that were most obvious to me.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:473
+    case Decl::CXXConstructor:
+    case Decl::CXXDestructor:
+      break;
----------------
lebedev.ri wrote:
> 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.
>> 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.

They can, but I wasn't certain if they'd add to complexity or not given that you can nest them deeply. But now I see that the paper doesn't mention them in terms of C++ but does talk about something similar with JavaScript that suggests they should not contribute to complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36836