[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 16 02:49:42 PDT 2017
JonasToth added a comment.
I built the patch locally, here is my output:
The Unittest failed for me: `Neither CHECK-FIXES nor CHECK-MESSAGES found in the input`
Script call:
`/usr/bin/python2.7 /home/jonas/opt/llvm/tools/clang/tools/extra/test/../test/clang-tidy/check_clang_tidy.py /home/jonas/opt/llvm/tools/clang/tools/extra/test/clang-tidy/readability-function-cognitive-complexity.cpp readability-function-cognitive-complexity /home/jonas/opt/llvm/build/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp -- -config='{CheckOptions: [{key: readability-function-cognitive-complexity.Threshold, value: 0}]}' -- -std=c++11`
Building the docs failed as well, i added inline comment on that.
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:62
+ std::pair<const char *, unsigned short> Process() const {
+ assert(C != Criteria::None && "invalid criteria");
+
----------------
lebedev.ri wrote:
> 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.
AFAIK `enum class` does not behave like a 'tagged integer'. It is necessary to to implement all operators (which you did), but i think implicit conversion are not allowed as well.
This would explain both the first and second error message.
Leave it as is, since the usage is limited to only this file.
================
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",
+
----------------
lebedev.ri wrote:
> 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];`
might the `static` cause that linking error?
Did you consider using `StringRef` instead of `const char*`? It is better behaved.
```constexpr std::array<StringRef, 4> Msgs = { /* messages */ };```
================
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) |
----------------
lebedev.ri wrote:
> 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
as said above, iam fine with that.
================
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;
----------------
lebedev.ri wrote:
> 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
I commented directly the macro on that. In my eyes the macro solution is Ok, but template function would be better ;)
For the `if(!ShouldContinue) ...` only a macro solution would come to mind. With a very descriptive name it might be acceptable, but others should decide on that.
Maybe `#define ReturnIfDone()...`, but idk.
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:206
+ /// common repetitive code is factored here.
+#define TraverseWithIncreasedNestingLevel(CLASS, Node) \
+ ++CurrentNestingLevel; \
----------------
Is macro the only possible way? Its certainly the fastest to implement, but i think an inline template function could to it as well (more to write, thats for sure). This function could return `ShouldContinue`.
I think some specialization would be necessary:
```
/// Traverses Stmt Nodes, default if explicitly called with different class
template <typename T> bool TraverseWithIncreasedNestingLevel(Node) {
/* Traversing here, with explict Base::Traverse...(Node) call */
}
/// For Decl
template<> bool TraverseWithIncreasedNestingLevel<Decl>(Node) {
// Same story, different call
}
```
Avoids the macro, produces more code. Current macro situation is already readable enough though.
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:420
+ }
+
+ /// In a sequence of binary logical operators, if new operator is different
----------------
undef the macros here?
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:441
+ }
+
+ /// It would make sense for the function call to start the new binary
----------------
undef the macros here? ( i mean after they were called)
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:616
+ for (const auto &Detail : Visitor.CC.Details) {
+ const char *IncreaseMessage = nullptr;
+ unsigned short Increase = 0;
----------------
Actually, this could be a `StringRef` as well.
================
Comment at: docs/ReleaseNotes.rst:138
+
+ Checks function Cognitive Complexity metric, and flags the functions with
+ Cognitive Complexity exceeding the configured limit.
----------------
lebedev.ri wrote:
> 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?
I struggle with it as well ;D
`Calculates the Cognitive Complexity metric for each function and flags those with a value exceeding the configured limit.`
I am not sure about the `flags those ...` part.
Swapping the beginning would make the sentence easier to read.
================
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`)
----------------
lebedev.ri wrote:
> 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.
The docs currently do not build for me, did you test them?
- `-DLLVM_ENABLE_SPHINX:BOOL=ON -DSPHINX_OUTPUT_HTML:BOOL=ON` in cmake
- `make docs-clang-tools-html` or ninja.
================
Comment at: docs/clang-tidy/checks/readability-function-cognitive-complexity.rst:30
+(by 1):
+ * Conditional operators:
+ * `if()`
----------------
readability-function-cognitive-complexity.rst:30:Unexpected indentation.
Same probably holds true on all other places here. I think if you remove the indenting of the list, everything plays out nicely.
Repository:
rL LLVM
https://reviews.llvm.org/D36836
More information about the cfe-commits
mailing list