[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:93-96
+    // Criteria only uses three bits, so uint8_t is used as an underlying type.
+    // We could make C a bitfield, but then we would not save anything because
+    // of the padding for alignment, and also we would have to worry about
+    // the differences between compilers.
----------------
I'm not certain this comment adds a whole lot.


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:154
+
+// All the possible messages that can be outputed. The choice of the message
+// to use is based of the combination of the CognitiveComplexity::Criteria's.
----------------
outputed -> output


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:155
+// All the possible messages that can be outputed. The choice of the message
+// to use is based of the combination of the CognitiveComplexity::Criteria's.
+// It would be nice to have it in CognitiveComplexity struct, but then it is
----------------
Criteria's -> Criteria


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:222
+
+  // Used to efficiently know the last type of binary sequence operator that
+  // was encountered. It would make sense for the function call to start the
----------------
of binary -> of the binary


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:367-368
+  bool TraverseStmt(Stmt *Node) {
+    if (!Node)
+      return Base::TraverseStmt(Node);
+
----------------
If there's not a node, do we really need to traverse it?


================
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:
----------------
`BinaryConditionalOperatorClass` as well (for all the places we're dealing with `ConditionalOperatorClass`)?


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


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:410
+    case Stmt::CXXCatchStmtClass:
+    case Stmt::LambdaExprClass:
+      Reasons |= CognitiveComplexity::Criteria::IncrementNesting;
----------------
`BlockExprClass` as well?

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


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:428
+    case Stmt::DoStmtClass:
+    case Stmt::CXXCatchStmtClass:
+      Reasons |= CognitiveComplexity::Criteria::PenalizeNesting;
----------------
`SEHExceptStmtClass` as well?


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


================
Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h:16-20
+namespace clang {
+namespace tidy {
+class ClangTidyContext;
+}
+} // namespace clang
----------------
This shouldn't be necessary, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36836