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