<div dir="ltr">Hi,<div><br></div><div>At least for the patches you link to, I cannot see any file using the SIMD module?</div><div>When it comes to warnings, please link to specific warnings/output rather than describing them :-)<br></div><div><br></div><div><br></div><div>I think there are at least two questions/issues to be aware of:</div><div><br></div><div>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 :-)</div><div><br></div><div><br></div><div>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.</div><div><br></div><div><br></div><div>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):</div><div><br></div><div>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 :-)</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>6) For documentation, you will have to explain the method/algorithm in the code rather than referring people to a long (?) scientific paper.</div><div><br></div><div><br></div><div><br></div><div>Cheers,</div><div><br></div><div>Erik</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div>Cheers,</div><div><br></div><div>Erik</div><div> </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 27, 2019 at 2:46 PM Ullmann, Thomas <<a href="mailto:thomas.ullmann@mpibpc.mpg.de">thomas.ullmann@mpibpc.mpg.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
we are getting warnings in Jenkins builds with the Clang static analysis tools<br>
for two patches that use the SIMD module. One category of warnings concerns<br>
the member vector of the Simd classes not being initialized in the default<br>
constructor of these classes. This time the warnings are not false positives,<br>
but I guess there is a reason for not initializing these members(?).<br>
The other concerns the way data in a union is accessed in scalar.h which is<br>
apparently not in line with cppcoreguidelines-pro-type-union-access.<br>
<br>
How should one deal with such warnings that pertain to preexisting code?<br>
<br>
The patches can be found here:<br>
<a href="https://gerrit.gromacs.org/#/c/4015/" rel="noreferrer" target="_blank">https://gerrit.gromacs.org/#/c/4015/</a><br>
<a href="https://gerrit.gromacs.org/#/c/3277/" rel="noreferrer" target="_blank">https://gerrit.gromacs.org/#/c/3277/</a><br>
<br>
Best,<br>
Thomas.<br>
-- <br>
Gromacs Developers mailing list<br>
<br>
* Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a> before posting!<br>
<br>
* Can't post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
<br>
* For (un)subscribe requests visit<br>
<a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a> or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div>Erik Lindahl <<a href="mailto:erik.lindahl@dbb.su.se" target="_blank">erik.lindahl@dbb.su.se</a>></div><div>Professor of Biophysics, Dept. Biochemistry & Biophysics, Stockholm University</div><div>Science for Life Laboratory, Box 1031, 17121 Solna, Sweden</div></div></div></div></div></div></div></div></div>