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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
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:
> > > > 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!
> `Msgs` is in a `struct CognitiveComplexity`, which is in anonymous namespace
Ups. But that solution wouldn't be that bad in term of structure size, would it? The messages themself should land in static program storage, so only 4 pointers would be saved. I would leave it as is, maybe make it `constexpr`.


Repository:
  rL LLVM

https://reviews.llvm.org/D36836