lebedev.ri added a comment.
In https://reviews.llvm.org/D36836#873246, @JonasToth wrote:
> For my part the current state is ok.
@JonasToth thank you for the review!
> but @alexfh and @aaron.ballman should do their review before committing.
+1 :)
Now what one full review is done, it may be easier to start for the other
reviewers..
> I would be interested in a exampleoutput for any real project.
TBD
================
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:
> > > 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`.
> But that solution wouldn't be that bad in term of structure size, would it?
I *think* so
> constexpr
Comment from before `const std::array<const StringRef, 4> Msgs = {{`
```
// Yes, this member variable would be better off being `static`. But then
// either `StringRef` does not have `constexpr` constructor (because `strlen`
is not
// `constexpr`), which is needed for `static constexpr` variable; or if
`char*` is
// used, there is a linking problem (Msgs is missing).
// So either static out-of-line or non-static in-line.
```
Repository:
rL LLVM
https://reviews.llvm.org/D36836
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
