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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
