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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM, thank you for your patience with the process.



================
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:
> > 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.
Agreed, this can be done in a follow-up.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:473
+    case Decl::CXXConstructor:
+    case Decl::CXXDestructor:
+      break;
----------------
lebedev.ri wrote:
> 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.
> What i mean is, can you define a new namespace within the function's body?

You cannot.

> I don't believe that to be possible, therefore we shouldn't/couldn't care about that, because we start at function scope.

Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36836