Code Quality: PVS Studio Finds #1901

thorstenhater · 2022-06-01T14:35:55Z

It's the season again when I put our code through the grinder and fix at least a few issues found.

arbor/memory/allocator.hpp

arbor/benchmark_cell_group.cpp

brenthuisman · 2022-06-03T06:53:15Z

arbor/s_expr.cpp

@@ -281,7 +281,7 @@ class lexer {
        }

        // test if the symbol matches a keyword
-        auto it = keyword_to_tok.find(symbol.c_str());
+        auto it = keyword_to_tok.find(symbol);


There are two kinds of people: those that append .c_str() gratuitously and those that really need it. How can PVSS tell which it is?

Find takes a const std::string& as std::string is the key type?!

.c_str() isn't a std::string?

It's a const char*, like in C. If std::string where a std::vector<char>, c_str would be like data, returning the underlying buffer by pointer.

Yes, but that's not what I meant. I guess the answer to my question is: yes, in this case PVSS knows how symbol is used and c_str() is not required for that use.

It knows since it can see this

std::map<K, V>::find(const K&)

and here K=std::string. We are calling

map.find(string.c_str())

so the temporary C string is converted back to a std::string.
That in turn is not possible without copying the bytes, so

not needed

wasteful

sup/include/sup/ioutil.hpp

brenthuisman · 2022-06-03T06:56:51Z

I have the feeling a lot of the errors are related to the assumption by PVSS that {} and = default are equal.

thorstenhater · 2022-06-05T15:15:20Z

I have the feeling a lot of the errors are related to the assumption by PVSS that {} and = default are equal.

But for the default ctor and dtor this is entirely correct, see nice answer here

https://stackoverflow.com/questions/20828907/the-new-syntax-default-in-c11?lq=1

The copy constructor is much more sketchy, but for empty classes it should be fine.

The errors are caused by something different, and they do not occur (neither a warning) without the sanitizers.
Fascinating.

brenthuisman · 2022-06-06T09:50:04Z

OK, so {} and = default are the same, apart from: "Giving a user-defined constructor, even though it does nothing, makes the type not an aggregate and also not trivial." The definitions of trivial and aggregate/POD seem awfully close btw., but if that's important, why not not declare the de-/constructor? Reading the above, = default gives you that behavior.

thorstenhater · 2022-06-07T09:47:28Z

If you declare any of the 5 methods (copy ctor, move ctor, copy=, move=, non-default ctor), the rest will not be auto-generated. But you can get them back by =default

brenthuisman · 2022-06-07T11:17:40Z

Ach so....

…ysis-22

thorstenhater · 2022-06-09T07:21:44Z

I have the feeling a lot of the errors are related to the assumption by PVSS that {} and = default are equal.

But for the default ctor and dtor this is entirely correct, see nice answer here

https://stackoverflow.com/questions/20828907/the-new-syntax-default-in-c11?lq=1

The copy constructor is much more sketchy, but for empty classes it should be fine.

The errors are caused by something different, and they do not occur (neither a warning) without the sanitizers. Fascinating.

So, the compile errors were cause by the SIMD classes declaring friend fma(SIMD, SIMD, SIMD), but not friend fma(const SIMD&, ...). That's all.

…ysis-22

brenthuisman · 2022-06-10T13:59:53Z

arbor/fvm_layout.cpp

@@ -248,32 +248,34 @@ ARB_ARBOR_API fvm_cv_discretization fvm_cv_discretize(const cable_cell& cell, co
    double dflt_potential =   *(dflt.init_membrane_potential | global_dflt.init_membrane_potential);
    double dflt_temperature = *(dflt.temperature_K | global_dflt.temperature_K);

+    const auto& assignments = cell.region_assignments();


I can't find where these are used (so I don't understand why they were added).

They are use in multiple places, yielding longish expressions, so we name it.

brenthuisman · 2022-06-10T14:08:34Z

arbor/include/arbor/s_expr.hpp

@@ -214,6 +214,7 @@ struct ARB_ARBOR_API s_expr {
    s_expr(s_expr l, s_expr r):
        state(pair_type(std::move(l), std::move(r)))
    {}
+    s_expr& operator=(const s_expr& s) { state = s.state; return *this; }


What's the algo for adding these? operator= must always be implemented or sth?

No, not really. The rule is to implement op= if you implement copy and vice veras. The argument is one of correctness:
If you need write one, it'll do something interesting. Therefore, you cannot use the default one.
You'll get a warning, but the result will be wrong, since the assignment operator will still be made for you see
https://gcc.godbolt.org/z/KfreGvxjY
Note especially the difference here

S a = S{}; // copy ctor, despite looking like assignment. S a; a = S{}; // assignment

brenthuisman · 2022-06-10T14:14:25Z

arbor/morph/place_pwlin.cpp

            const p3d vw = w-v;
-            const p3d vp = p-v;


Strange to remove one but not both. Any idea why?

vp is constructed inside a conditional and closer to its sole site of use. So, it's clearer and it might save the odd cycle.

brenthuisman

I went through the whole thing, looks reasonable to me. Have a few more questions, see above, but they are about PVSS's choices, not the code itself.

thorstenhater · 2022-06-11T07:49:25Z

Most of them are reasonable things to admonish, although I ignored about 60% of the finds.
I also excluded the directories python, test, modcc, and example outright.
Next time, we can do some more.

Here's a suggestion: Ask PVS for a license (free for FOSS), run it and go over the results.
I can give you my filter scripts. Each has a key, which you can index into their exaplanation DB.
Or you can just ask (me).

thorstenhater added 5 commits June 1, 2022 11:02

Use = default where possible.

2765146

Call base class virtual functions where needed for clarity.

10bae82

Remove redundante compare. Length is unsigned.

ce40536

Use const& where possible.

021f31b

Redundant cast.

0e5e302

brenthuisman reviewed Jun 3, 2022

View reviewed changes

arbor/memory/allocator.hpp Show resolved Hide resolved

brenthuisman reviewed Jun 3, 2022

View reviewed changes

arbor/benchmark_cell_group.cpp Show resolved Hide resolved

brenthuisman reviewed Jun 3, 2022

View reviewed changes

sup/include/sup/ioutil.hpp Show resolved Hide resolved

thorstenhater added 12 commits June 8, 2022 10:06

Merge remote-tracking branch 'origin/master' into quality/static-anal…

bf1a146

…ysis-22

Fix friendship.

d4dae51

Init the colours.

fe51e07

Clean-up fvm_layout.

28d2a25

Properly use move semantics.

f05e5ce

More move semantics.

dc7cbf2

Micro-optimisations.

e096d12

Small fixes in mechcat.

2b433ce

Use const& in cable io.

abbfd78

Micro-optimisation grab-bag.

377d104

Avoid re-alloc.

051966a

Polish thread storage.

0f9a187

thorstenhater requested review from AdhocMan, boeschf, brenthuisman and lukasgd June 9, 2022 07:20

Merge remote-tracking branch 'origin/master' into quality/static-anal…

52a535d

…ysis-22

brenthuisman reviewed Jun 10, 2022

View reviewed changes

brenthuisman approved these changes Jun 10, 2022

View reviewed changes

thorstenhater merged commit 14f2d6a into arbor-sim:master Jun 13, 2022

thorstenhater deleted the quality/static-analysis-22 branch August 1, 2022 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Quality: PVS Studio Finds #1901

Code Quality: PVS Studio Finds #1901

thorstenhater commented Jun 1, 2022

brenthuisman Jun 3, 2022

thorstenhater Jun 3, 2022

brenthuisman Jun 6, 2022

thorstenhater Jun 7, 2022

brenthuisman Jun 10, 2022

thorstenhater Jun 11, 2022

brenthuisman commented Jun 3, 2022

thorstenhater commented Jun 5, 2022 •

edited

Loading

brenthuisman commented Jun 6, 2022

thorstenhater commented Jun 7, 2022

brenthuisman commented Jun 7, 2022

thorstenhater commented Jun 9, 2022

brenthuisman Jun 10, 2022

thorstenhater Jun 11, 2022

brenthuisman Jun 10, 2022

thorstenhater Jun 11, 2022

brenthuisman Jun 10, 2022

thorstenhater Jun 11, 2022

brenthuisman left a comment

thorstenhater commented Jun 11, 2022 •

edited

Loading

Code Quality: PVS Studio Finds #1901

Code Quality: PVS Studio Finds #1901

Conversation

thorstenhater commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brenthuisman commented Jun 3, 2022

thorstenhater commented Jun 5, 2022 • edited Loading

brenthuisman commented Jun 6, 2022

thorstenhater commented Jun 7, 2022

brenthuisman commented Jun 7, 2022

thorstenhater commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brenthuisman left a comment

Choose a reason for hiding this comment

thorstenhater commented Jun 11, 2022 • edited Loading

thorstenhater commented Jun 5, 2022 •

edited

Loading

thorstenhater commented Jun 11, 2022 •

edited

Loading