lebedev.ri 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", + ---------------- 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? ================ 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: > 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. Actually, it turned out to be much simper now that i cross-referenced with the output of the "reference implementation" ================ Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:206 + /// common repetitive code is factored here. +#define TraverseWithIncreasedNestingLevel(CLASS, Node) \ + ++CurrentNestingLevel; \ ---------------- JonasToth wrote: > 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. Modified, but without `template`s. ================ Comment at: docs/ReleaseNotes.rst:138 + + Checks function Cognitive Complexity metric, and flags the functions with + Cognitive Complexity exceeding the configured limit. ---------------- 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 ================ 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: > 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. > did you test them? No, actually. Thanks for the hint on how to build them, now fixed. Repository: rL LLVM https://reviews.llvm.org/D36836 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits