Use cppcheck with WorkDir, master branch (2022.01.11.)

Hey @akraszna . Thanks for this.

The question is perhaps more for @sroe (and perhaps @ssnyder)

@sroe do we want just "error" or we want also to have "warning" i.e have --enable=warning also in the command line. Default would be error only .

I would avoid more that that until we get some experience, but warning is part of the stats you present https://twiki.cern.ch/twiki/pub/AtlasComputing/CppCheck/cppcheck.png

Hi, yes warning is also worthwhile. As you have seen above, I count error, warning and portability. I think portability is also useful; it typically catches things like integer overflow when shifting by too many bits (maybe developer used signed instead of unsigned int) which in my view is still a 'real' bug.

added 1385 commits

d9472738...930abcf9 - 1384 commits from branch atlas:master
025b89a0 - Taught WorkDir how to make use of cppcheck if it's available.

Compare with previous version

marked this merge request as ready

This merge request affects 1 package:

Projects/WorkDir

This merge request affects 1 file:

Projects/WorkDir/CMakeLists.txt

Adding @akraszna as watcher

added Build master review-pending-level-1 labels

Let me take this out of draft status, now that Christos reminded me about it...

The cppcheck executable has been removed from AthenaExternals a while ago. So if we add this, cppcheck will only do anything in a "WorkDir build" if the user explicitly sets up cppcheck themselves. Like with:

[bash][atlas]:build > lsetup "cppcheck testing"
************************************************************************
Requested:  cppcheck ... 
 Setting up cppcheck 2.6-x86_64-centos7-gcc11 ... 
>>>>>>>>>>>>>>>>>>>>>>>>> Information for user <<<<<<<<<<<<<<<<<<<<<<<<<
 cppcheck:
   To use cppcheck you need to setup a release or a nightly first
************************************************************************
[bash][atlas]:build >

Note that this simple setup triggers a check on the generated sources (most notably on dictionaries) as well. Which do not seem to see eye to eye with cppcheck at the moment...

/data/hdd-4tb/projects/cppcheck/build/Event/xAOD/xAODCore/CMakeFiles/xAODCoreCintDict.cxx:302:16: error: AST broken, ternary operator missing operand(s) [internalAstError]
      return p ? ::new((::ROOT::Internal::TOperatorNewHelper*)p) pair<string,xAOD::BranchStats>[nElements] : new pair<string,xAOD::BranchStats>[nElements];
               ^
data/hdd-4tb/projects/cppcheck/build/Event/xAOD/xAODCore/CMakeFiles/xAODCoreSTLDictReflexDict.cxx:64:38: error: syntax error: 64 = [syntaxError]
   static ::ROOT::TGenericClassInfo *_R__UNIQUE_DICT_(Init) = GenerateInitInstanceLocal((const ::__pair_base<string,vector<float> >*)0x0); R__UseDummy(_R__UNIQUE_DICT_(Init));
                                     ^

Still, we could say that as a start, if somebody explicitly sets up cppcheck for their build, they should be aware that false positives may show up. Or do you have a suggestion for how to avoid warnings from files that have a certain pattern in their name?

Hi the root Dict is also a bit of an issue with clang-tidy ....

I assume this syntax would work or at least this is what we kind of use in the twiki.

--suppress=assignmentInAssert:*_test.cxx 
--suppress=assertWithSideEffect:*_test.cxx 
--suppress=*:*Dict.cxx

Again @ssnyder and @sroe might know more

That does work. I assume the other suppressions are for GoogleTest or Boost Test formalisms. Should we suppress those by default as well?

Remember, this setup is just for users to get a convenient view of what errors cppcheck triggers. It will not turn on anything for the nightly/CI build. So showing a bit too much rather than a bit too little would be my choice.

yea

Now the issue is that we have 4 flavour in the twiki ... This why @ssnyder and @sroe should comment ;)

This seems to be there for all "flavours"

--enable=warning,portability
--inline-suppr 
--template=gcc 
-D__CPPCHECK__ -DATOMIC_POINTER_LOCK_FREE=2

Enabling warning and portability.
The template is how the result is presented .
The inline-suppr suppresses things that have this in code // cppcheck-suppress "suppressed_error_id " [https://acode-browser1.usatlas.bnl.gov/lxr/search?%21v=head&_filestring=&_string=cppcheck-suppress]

@ssnyder seems to add

--suppress=assignmentInAssert:*_test.cxx 
--suppress=assertWithSideEffect:*_test.cxx

I think he also uses

--suppress=accessMoved:*_test.cxx

here. And all these would be useful for tests.

So all the above might actually make sense to have.

For then on :

One could also enable performance (@sroe and @ssnyder seem to do ) but then the number one and two issues will become:

Prefix ++/-- operators should be preferred for non-primitive types. Pre-increment/decrement can be more efficient than post-increment/decrement. Post-increment/decrement usually involves keeping a copy of the previous value around and adds a little extra code

Parameter is passed by value. It could be passed as a const reference which is usually faster and recommended in C++

And this might or might not be "noise" at this level.

At the end given a use pool of 3-4 people I would guess we can start somewhere and take it from there... I recall also @fwinkl had some ideas... ?

added 1 commit

0bef3cc3 - Taught WorkDir how to make use of cppcheck if it's available.

Compare with previous version

This merge request affects 1 package:

Projects/WorkDir

This merge request affects 1 file:

Projects/WorkDir/CMakeLists.txt

Adding @akraszna as watcher

Okay, let me then propose this list for now...

I see the value in those suppressions, but I don't want to put them in by default. At this point I'd really just want to suppress stuff that we can really not do anything about. (Without very significant effort.) All those test specific suppressions are fine for individual developers to use, but I would just show them by default for now.

resolved all threads

CI Result SUCCESS (hash 025b89a0)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis	DetCommon
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
DetCommon: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 46458]

CI Result SUCCESS (hash 0bef3cc3)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis	DetCommon
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
DetCommon: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 46462]

BTW, i have a few patches to cppcheck to fix some crashes and false positives which i've been meaning to get around to submitting upstream.

https://github.com/scott-snyder/cppcheck/tree/sss

added review-approved label and removed review-pending-level-1 label

Use cppcheck with WorkDir, master branch (2022.01.11.)

Merge request reports

Activity