Thanks for the feedback. I’ve made some comments below.
On Sep 19, 2017, at 3:17 AM, Damian Maguire <dmagdrums@...
Great to get your feedback here, and welcome to the dev list. Absolutely the right place for this sort of discussion - GitHub is I think are more geared towards individual issues rather than larger project discussions such as this.
Myself and Frank have spent a good bit of time discussing the subject of static analysis in the OpenMAMA codebase actually (some of the other contributors I think are aware of my interest in that area), and I think the community as a whole would be more than happy to see a stronger focus on it going forward. When support was added for building with Clang, we actually added native support for the Clang static analysis tools via scan-build - https://github.com/OpenMAMA/OpenMAMA/blob/master/site_scons/site_tools/scan-build.py
- which means you can actually build and analyse within a single step:
scan-build --use-c=/path/to/clang --use-cxx=/path/to/clang++ --use-analyzer=/path/to/clang scons ...
I'd love to see similar integration with other tools going forward (including CPPCheck, and the Google Santizer Suite), so if you have experience in that area I'd be happy to review any contributions.
Right now, getting static analysis working with other tools is somewhat dependent on the build tooling. I use a custom build script that runs scons under bear to create the compilation database required by my scripts. The build script is somewhat specific to my environment, so I don’t know how useful it would be to others. but I’ve attached it just in case.
However, once you’ve got a compilation database built (and cppcheck installed), you can use the scripts at https://github.com/btorpey/static
to run analysis using both clang and cppcheck, e.g.:
cc_cppcheck.sh -i common/c_cpp/src/c/ -i mama/c_cpp/src/c/ -c 2>&1 | tee cppcheck.csv
Similar scripts exist for clang-check and clang-tidy (although afaict clang-tidy does everything that clang-check does and more).
Regarding any fixes you want to submit, I say get them raised and get them in - given Frank's Windows warning hunt is coming to an end (for now) tackling some of these feels like a natural next step.
I guess I’m curious about what others think about relative priorities, and/or whether there is are natural “owners” for different parts of the code-base that might want to have input.
The only thing I would add, and related to your comment about the scope problems, is any changes will need to take into account the breadth of compilers OpenMAMA has to support and that unfortunately means continuing with the C89/C99 cross breed that older versions of Visual Studio rely on. This isn't so much an OpenMAMA stylistic choice as one that's been forced on us by compiler support, but the wider user community relies on us continuing to adhere to this as a standard (for now ;-)).
In the meantime, it’s easy enough to filter out those warnings.
Great to hear you're getting to use OpenMAMA as part of your day job as well. Any feedback you have about your experience the community will be delighted to hear, including in the form of blog posts, so work away. If you want someone to review, or have any questions, I'm happy to help as well.
Looking forward to seeing more of your work.
I've been working with OpenMAMA for a little while now as part of my "day job", and I've also been working with static analysis tools for some time, so I thought it would be fun to put the two together.
As a first step, I ran the OpenMAMA code through cppcheck, which has become my go-to tool for static analysis, and produced the attached reports. (FWIW, clang is good too, but I find that cppcheck flags more suspect code, and for me more is usually better).
I'm not sure what the rest of the community thinks about the importance of static analysis -- personally I'm a big believer, and I'm happy to get on board with helping to resolve some of these if others agree that would be a Good Thing.
For the record, the analysis was done on the 6.2.1 release, using cppcheck version 1.80, and using the scripts I've previously published as part of an ongoing series of articles about static analysis on [my blog](http://btorpey.github.io/). The specific command used was:
cc_cppcheck.sh 2>&1 | grep -v '/cpp' | grep -v test | grep -v examples | grep -v stdout | tee cppcheck-180.out
The idea was to concentrate on the core OpenMAMA C code, so the command explicitly ignores C++ code, test and examples. (As well as some mysterious occurences of `stdout` which I haven't had time to track down yet).
A few observations:
- At least one of these issues has been [fixed subsequent to the release](https://github.com/OpenMAMA/OpenMAMA/pull/310).
- There are a boat-load of "scope can be reduced" warnings -- in most cases these can probably be explained by the K&R-style of declarations that OpenMAMA uses as a standard. (Unfortunately, in my opinion ;-(
- Similarly, there are a bunch of "reassigned a value before the old one has been used", likely with the same cause.
- There are also a bunch of "argument ... names different" warnings -- at least one of these has [bitten me in the past](https://github.com/OpenMAMA/OpenMAMA/issues/297).
As mentioned above, I'm very interested in the community's thoughts on where to go from here (if anywhere).
P.S. I'm posting this to the mailing list, rather than to the GitHub issues, but please let me know if you think that would be a more appropriate place.
P.P.S. I'm also writing another article in my series on static analysis in which I'll be using this analysis to discuss the benefits of static analysis in general, and cppcheck in particular. This should in no way be seen as a criticism of the OpenMAMA code -- I've been working with OpenMama (as well as DataFabric) for quite a while, and it's a truly unique and impressive effort. So, kudos to all for making OpenMAMA what it is -- but if we can make it even better, that's to everyone's benefit.
Openmama-dev mailing list