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