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