[gmx-developers] How to deal with Clang-tidy/Clang static analyzer warnings in existing headers?

[Erik Lindahl]

Hi,

At least for the patches you link to, I cannot see any file using the SIMD
module?
When it comes to warnings, please link to specific warnings/output rather
than describing them :-)


I think there are at least two questions/issues to be aware of:

1) When it comes to using the SIMD module, one has to be aware it's very
much a compromise. It leads to code that can be VERY difficult to
adjust/maintain/understand, occasionally double implementations
(with/without SIMD), and at least the problem that anybody editing it in
the future will have to be aware of SIMD. This is a price that might be
worth it for certain core parts of the code where we collectively spend
millions of core-hours on large computers, but IMHO it should not be used
for analysis tools, even it it might create a nice speedup.   When it comes
to SIMD code an unions, I suspect you are hitting warnings related to
conversions between floating-point and bitwise representations. This
requires a round-trip to memory in C++, which is not strictly guaranteed by
the standard (IEEE754 is not a requirement of C++). This is another reason
for not touching the SIMD parts of the code unless you are working with
very deep plumbing :-)


2) When it comes to errors in old-vs.-new parts of the code it's fairly
easy: Since the master branch passes verification, any errors in old code
will have been in parts that were not used prior to the new change. That is
not ideal, but it might happen. Effectively, we treat that the same way as
new code (since it is newly *used* code) - it has to be fixed before the
change can go in.


To get the change you link to in shape to be commited, here are my general
comments (just based in 5 minutes looking at it):

1) Forget SIMD. Clean up the patch and get it working 100% first. If the
most common complaint among GROMACS users a year from now is that
functional mode analysis needs to be faster, then we'll consider
acceleration :-)

2) Document the classes carefully. How should the class be *used* (not
merely that it implements a method), and break them apart into small
manageable classes. Aim at max ~5 data members per class, not 25-50. Make
sure you specify what the allowed input/parameters are, and what will
happen if somebody provides illegal values.

3) Same thing for member classes - aim for functions that are 5-10 lines,
and in worst case 25-50. No function in a new piece of code should be 200+
lines-of-code, because it becomes impossible to understand and test
everything that function does. Second, make sure the code is documented
inside those routines too - because the person doing code review needs to
understand the code, not merely pray the compiler found the errors and hope
the first user knew what (s)he was doing.

4) Use C++ iteration constructs to avoid lots of loop variables, and avoid
things like manual vector resizes unless absolute necessary. It is often
better to let something go out of scope and create a new vector, unless it
has unacceptable performance implications. Readability & maintainability
trumps performance.

5) Several routines in the changes appear to work with raw pointers, which
is a fairly big no-no for new code since we know we're not smart enough to
avoid causing memory errors. Stick to STL datatypes & iterators.

6) For documentation, you will have to explain the method/algorithm in the
code rather than referring people to a long (?) scientific paper.



Cheers,

Erik





Cheers,

Erik


On Wed, Feb 27, 2019 at 2:46 PM Ullmann, Thomas <
thomas.ullmann at mpibpc.mpg.de> wrote:

> Hi,
>
> we are getting warnings in Jenkins builds with the Clang static analysis
> tools
> for two patches that use the SIMD module. One category of warnings concerns
> the member vector of the Simd classes not being initialized in the default
> constructor of these classes. This time the warnings are not false
> positives,
> but I guess there is a reason for not initializing these members(?).
> The other concerns the way data in a union is accessed in scalar.h which is
> apparently not in line with cppcoreguidelines-pro-type-union-access.
>
> How should one deal with such warnings that pertain to preexisting code?
>
> The patches can be found here:
> https://gerrit.gromacs.org/#/c/4015/
> https://gerrit.gromacs.org/#/c/3277/
>
> Best,
> Thomas.
> --
> Gromacs Developers mailing list
>
> * Please search the archive at
> http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List before
> posting!
>
> * Can't post? Read http://www.gromacs.org/Support/Mailing_Lists
>
> * For (un)subscribe requests visit
> https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers
> or send a mail to gmx-developers-request at gromacs.org.
>


-- 
Erik Lindahl <erik.lindahl at dbb.su.se>
Professor of Biophysics, Dept. Biochemistry & Biophysics, Stockholm
University
Science for Life Laboratory, Box 1031, 17121 Solna, Sweden
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://maillist.sys.kth.se/pipermail/gromacs.org_gmx-developers/attachments/20190227/243469c2/attachment.html>