[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check
Roman Lebedev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 15 13:53:52 PDT 2017
lebedev.ri added inline comments.
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:62
+ std::pair<const char *, unsigned short> Process() const {
+ assert(C != Criteria::None && "invalid criteria");
+
----------------
JonasToth wrote:
> You acces `Critera` always scoped. I think you could declare `Criteria` to be a `enum class`, not just a plain `enum`
Apparently it's not as simple as that...
Then i get:
```
1/8] Building CXX object tools/clang/tools/extra/clang-tidy/readability/CMakeFiles/clangTidyReadabilityModule.dir/FunctionCognitiveComplexityCheck.cpp.o
FAILED: tools/clang/tools/extra/clang-tidy/readability/CMakeFiles/clangTidyReadabilityModule.dir/FunctionCognitiveComplexityCheck.cpp.o
/usr/local/bin/clang++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clang-tidy/readability -I/build/clang-tools-extra/clang-tidy/readability -I/build/clang/include -Itools/clang/include -I/usr/include/libxml2 -Iinclude -I/build/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -fno-exceptions -fno-rtti -MD -MT tools/clang/tools/extra/clang-tidy/readability/CMakeFiles/clangTidyReadabilityModule.dir/FunctionCognitiveComplexityCheck.cpp.o -MF tools/clang/tools/extra/clang-tidy/readability/CMakeFiles/clangTidyReadabilityModule.dir/FunctionCognitiveComplexityCheck.cpp.o.d -o tools/clang/tools/extra/clang-tidy/readability/CMakeFiles/clangTidyReadabilityModule.dir/FunctionCognitiveComplexityCheck.cpp.o -c /build/clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
/build/clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:100:44: error: invalid operands to binary expression ('clang::tidy::readability::(anonymous namespace)::CognitiveComplexity::Criteria' and 'clang::tidy::readability::(anonymous namespace)::CognitiveComplexity::Criteria')
} else if (C == (Criteria::Increment | Criteria::IncrementNesting)) {
~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:536:9: error: value of type 'CognitiveComplexity::Criteria' is not contextually convertible to 'bool'
if (Reasons & CognitiveComplexity::Criteria::All)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
ninja: build stopped: subcommand failed.
```
The first failure i could have explained as use-before-declaration. But not the second one
I'll keep it as is where it is for now.
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114
+const std::array<const char *const, 4> CognitiveComplexity::Msgs = {{
+ // B1 + B2 + B3
+ "+%0, including nesting penalty of %1, nesting level increased to %2",
+
+ // B1 + B2
+ "+%0, nesting level increased to %2",
+
----------------
JonasToth wrote:
> could this initialization land in line 45? that would be directly close to the criteria.
It would be nice indeed, but if i do
```
// All the possible messages that can be outputed. The choice of the message
// to use is based of the combination of the Criterias
static constexpr std::array<const char *const, 4> Msgs = {{
// B1 + B2 + B3
"+%0, including nesting penalty of %1, nesting level increased to %2",
// B1 + B2
"+%0, nesting level increased to %2",
// B1
"+%0",
// B2
"nesting level increased to %2",
}};
```
i get
```
$ ninja check-clang-tools -j1
[1/5] Linking CXX executable bin/clang-tidy
FAILED: bin/clang-tidy
: && /usr/local/bin/clang++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -Wl,-allow-shlib-undefined -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clang-tidy/tool/CMakeFiles/clang-tidy.dir/ClangTidyMain.cpp.o -o bin/clang-tidy -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.a -lpthread lib/libclangAST.a lib/libclangASTMatchers.a lib/libclangBasic.a lib/libclangTidy.a lib/libclangTidyAndroidModule.a lib/libclangTidyBoostModule.a lib/libclangTidyBugproneModule.a lib/libclangTidyCERTModule.a lib/libclangTidyCppCoreGuidelinesModule.a lib/libclangTidyGoogleModule.a lib/libclangTidyHICPPModule.a lib/libclangTidyLLVMModule.a lib/libclangTidyMiscModule.a lib/libclangTidyModernizeModule.a lib/libclangTidyMPIModule.a lib/libclangTidyPerformanceModule.a lib/libclangTidyReadabilityModule.a lib/libclangTooling.a lib/libclangToolingCore.a lib/libclangTidyCppCoreGuidelinesModule.a lib/libclangTidyGoogleModule.a lib/libclangTidyMiscModule.a lib/libclangTidyReadabilityModule.a lib/libclangTidyUtils.a lib/libclangTidy.a lib/libclangTooling.a lib/libclangFormat.a lib/libclangToolingCore.a lib/libclangStaticAnalyzerFrontend.a lib/libclangFrontend.a lib/libclangDriver.a lib/libLLVMOption.a lib/libclangParse.a lib/libLLVMMCParser.a lib/libclangSerialization.a lib/libclangSema.a lib/libclangEdit.a lib/libLLVMBitReader.a lib/libLLVMProfileData.a lib/libclangStaticAnalyzerCheckers.a lib/libclangStaticAnalyzerCore.a lib/libclangASTMatchers.a lib/libclangRewrite.a lib/libclangAnalysis.a lib/libclangAST.a lib/libclangLex.a lib/libclangBasic.a lib/libLLVMCore.a lib/libLLVMBinaryFormat.a lib/libLLVMMC.a lib/libLLVMSupport.a -lrt -ldl -ltinfo -lpthread -lz -lm lib/libLLVMDemangle.a && :
lib/libclangTidyReadabilityModule.a(FunctionCognitiveComplexityCheck.cpp.o):FunctionCognitiveComplexityCheck.cpp:function clang::tidy::readability::FunctionCognitiveComplexityCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&): error: undefined reference to 'clang::tidy::readability::(anonymous namespace)::CognitiveComplexity::Msgs'
```
Same if `process()` returns `std::pair<unsigned, unsigned short>` and in `FunctionCognitiveComplexityCheck::check()` i do `const char *IncreaseMessage = Visitor.CC.Msgs[IncreaseMsgId];`
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:116-146
+// Criteria is a bitset, thus a few helpers are needed
+static CognitiveComplexity::Criteria
+operator|(CognitiveComplexity::Criteria lhs,
+ CognitiveComplexity::Criteria rhs) {
+ return static_cast<CognitiveComplexity::Criteria>(
+ static_cast<std::underlying_type<CognitiveComplexity::Criteria>::type>(
+ lhs) |
----------------
JonasToth wrote:
> `Criteria` could be moved out of the `Detail` struct. That would allow closer definiton of the helper code to the `enum`.
`enum Criteria` is in `struct CognitiveComplexity`.
I personally think it makes most sense there, since the criteria is about cognitive complexity
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:154
+ Details.emplace_back(Loc, Nesting, C);
+ const Detail &d(Details.back());
+
----------------
JonasToth wrote:
> s/d/D/ naming convention
Since i use a flat git repo layout, there is no `.clang-tidy` file, so i missed this.
Now corrected all valid `clang-tidy` complaints about this file.
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:168
+ // Used to efficiently know the last type of binary sequence operator that
+ // was encoutered. It would make sense for the function call to start the
+ // new sequence, thus it is a stack.
----------------
JonasToth wrote:
> s/encoutered/encountered/
Too bad qtcreator still does not appear to have an integrated spell checker :(
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:228-234
+ // if increases nesting level
+ ++CurrentNestingLevel;
+ ShouldContinue = TraverseStmt(Node->getThen());
+ --CurrentNestingLevel;
+
+ if (!ShouldContinue || !Node->getElse())
+ return ShouldContinue;
----------------
JonasToth wrote:
> line 228-234 are a pattern, existing in all traversals. i think it could be refactored into its own function with a descriptive name. Especially the increment, traverse, decrement isn't obvious immediatly, but when noting its blockyness it is.
Macro hell, here i come!!1
Is this any better? :)
I did not move `if (!ShouldContinue) return ShouldContinue;` into the macro, i think it would be totally bad then
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:507
+
+ bool TraverseDecl(Decl *Node, bool MainAnalyzedFunction = false) {
+ if (!Node || MainAnalyzedFunction)
----------------
JonasToth wrote:
> I assume this is the part applicable to class complexity? Maybe a short comment would clarify.
Yeah, indeed i should have documented it
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:584
+ for (const auto &Detail : Visitor.CC.Details) {
+ const std::pair<const char *, unsigned short> Reasoning(Detail.Process());
+ diag(Detail.Loc, Reasoning.first, DiagnosticIDs::Note)
----------------
JonasToth wrote:
> What would you think about the following:
> ```
> const char *IncreaseMessage = nullptr;
> unsigned short Increase = 0;
> std::tie(IncreaseMessage, Increase) = Reasoning(Detail.Process());
> ```
>
> Would be applicable elsewhere too. It's more going to the C++17 structured bindings and good potential target for futurue `modernize-` checks ;)
> And i think it would be more readable.
Hmm, yeah, looks somewhat better indeed, thanks.
================
Comment at: docs/ReleaseNotes.rst:138
+
+ Checks function Cognitive Complexity metric, and flags the functions with
+ Cognitive Complexity exceeding the configured limit.
----------------
JonasToth wrote:
> How about
> 'Applies the Cognitive Complexity metric to functions (classes?), flagging those exceeding the configured limit/threshold'
English is hard :P
Reworded, maybe better?
================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:10
+<https://www.sonarsource.com/docs/CognitiveComplexity.pdf>`_ specification
+version 1.2 (19 April 2017), with two notable exceptions:
+ * `preprocessor conditionals` (`#ifdef`, `#if`, `#elif`, `#else`, `#endif`)
----------------
JonasToth wrote:
> One or two sentences for the general thought would be nice for the quick reader, who doesn't like pdfs.
>
> Noting, that the metric adds additional penalties for nesting and that switch cases are not the same complexity as if/else-if chains would be enough.
Wrote something, not sure how well it will be rendered in HTML.
================
Comment at: test/clang-tidy/readability-function-cognitive-complexity.cpp:23
+// in one go. none of the following should increase cognitive complexity:
+void unittest_false() {
+ {};
----------------
JonasToth wrote:
> What happens in a codeblock like this:
>
> ```
> void f() {
> { // nesting is increased, but not complexity, right? There is no occurence of a 'count' event
> throw i;
> }
> }
> ```
Nothing happens, `{}` does not increase nesting.
Added this testcase to `unittest_false()`.
================
Comment at: test/clang-tidy/readability-function-cognitive-complexity.cpp:481
+// CHECK-NOTES: :[[@LINE-1]]:13: note: +2, including nesting penalty of 1, nesting level increased to 2{{$}}
+// FIXME: would be nice to point at the '?' symbol. does not seem to be possible
+ }
----------------
JonasToth wrote:
> did you try to get `getTrueExpr`/`getFalseExpr` and point to their respective end/beginning?
Oh nice, `ConditionalOperator` already has `getQuestionLoc()` from `AbstractConditionalOperator`!
Repository:
rL LLVM
https://reviews.llvm.org/D36836
More information about the cfe-commits
mailing list