Re: OpenMAMA static analysis


Frank Quinn <fquinn.ni@...>
 

The question of support for visual studio is one for the community which I'll actually pose on a separate thread with its own subject so that people might read it, but since VS2013 only went EOL from microsoft a year ago, I expect there are a few out there still using it.

Last time we asked, there were still some 2008 users out there too. As you can see, we actually only got rid of 2005 support as recently as 2 years ago (I was keen to do it as 2008 introduced proper msbuild support so we could finally upgrade to newer visual studio project formats). I'm keen to move forward too, but I don't want to alienate our client base either, so any movement forward would need to have 100% community support.

Generally for static code analysis, I'm all for it - it tends to prevent issues that weren't even considered. However it would be a lower priority for me behind a few other outstanding CI tasks related to making us more comfortable in merging changes, namely:
  • CI and removal of warnings for Java builds
  • Unit tests in CI environment for C#
  • Windows CI for pull requests (possibly appveyor)
  • Set up of jenkins multi platform builds for windows and linux (which is now possible since they both use the same CI script)
It does continue the gradual increase in sophistication for CI we have had in the last couple of years though... first of all we got unit tests working, then we got warnings removed on linux, now they're gone on windows, but yes I definitely want to always be moving towards a better CI environment which includes static analysis... but they tend to be slow burning tasks so it might take a while to be part of our day-to-day maintenance, but I'd happily accept any static code analysis related changes in the interim.

Cheers,
Frank

On Tue, Sep 19, 2017 at 2:08 PM, Bill Torpey <wallstprog@...> wrote:
Hi Damian: 

Thanks for the feedback.  I’ve made some comments below.

Regards,

Bill

On Sep 19, 2017, at 3:17 AM, Damian Maguire <dmagdrums@...> wrote:

Hey Bill, 

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 ;-)).  

Curious if there’s a timetable for requiring VS2013 (the first MS compiler that doesn’t have this problem).  As you’re probably aware, the K&R style is “officially” frowned on, at least in the C++ Core Guidelines (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es21-dont-introduce-a-variable-or-constant-before-you-need-to-use-it).  I notice that even some of Frank’s side projects (zmq, omnm) deviate from K&R style, which is understandable given how archaic it is.

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. 

Thanks.  I’ll send out a link when the article is complete — certainly interested in any feedback.  In the meantime, you may want to check out the other articles in the series: http://btorpey.github.io/blog/categories/static-analysis/


Looking forward to seeing more of your work. 

You’ve already seen some, just under another moniker: https://github.com/pulls?utf8=%E2%9C%93&q=author%3Abill-torpey-ullink+








Thanks, 

Damian 

On Sat, Sep 16, 2017 at 7:27 PM Bill Torpey <wallstprog@...> wrote:
Hi all:

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).

Regards,

Bill Torpey

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
Openmama-dev@....org
https://lists.openmama.org/mailman/listinfo/openmama-dev


_______________________________________________
Openmama-dev mailing list
Openmama-dev@....org
https://lists.openmama.org/mailman/listinfo/openmama-dev


Join Openmama-dev@lists.openmama.org to automatically receive all group messages.