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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 4 inline comments as done.
lebedev.ri added inline comments.


================
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:
> 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.
As discussed, it does seem like they should at least cause increase in nesting level.
I suspect, they should also cause increase in cognitive complexity IFF they're nested somehow,
because `foo ?: bar?: baz` doesn't really seem straight-forward.
But likewise, they're an extension, so i think we could fine-tune that later.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:428
+    case Stmt::DoStmtClass:
+    case Stmt::CXXCatchStmtClass:
+      Reasons |= CognitiveComplexity::Criteria::PenalizeNesting;
----------------
aaron.ballman wrote:
> 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.
Okay, thanks.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:473
+    case Decl::CXXConstructor:
+    case Decl::CXXDestructor:
+      break;
----------------
aaron.ballman wrote:
> 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.
What i mean is, can you define a new `namespace` within the function's body?
I don't believe that to be possible, therefore we shouldn't/couldn't care about that,
because we start at function scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36836