[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

For my part the current state is ok. @alexfh and @aaron.ballman but should do their review before committing.
I would be interested in a exampleoutput for any real project.



================
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:
> > 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 */ };```
> Actually, found a combination that works, done.
> FIXME: so should `const std::array<const StringRef, 4> CognitiveComplexity::Msgs` be `static` and initialized out-of-line, or not `static`, but initialized in-line?
Since it is already in an anonymous namespace, static would be duplicated, wouldn't it? I like it now!


================
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:
> > 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.
> Reworded once more
Sounds good. :)


Repository:
  rL LLVM

https://reviews.llvm.org/D36836