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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
