False positive warning PVS Studio: V821 Decreased perfomance. The 'rhs' variable can be constructed in a lower level scope

Question

PVS Studio 6.17 (Windows 7, 64Bit, VS2017, C++-03) seems to give a wrong warning on following reduced code

#include <stack>
#include <string>
#include <vector>
bool fred(const std::string &x)
{
    return x == "ab";
}
std::vector<std::string> bar(std::stack<std::string> & s)
{
    std::vector<std::string> v;
    const std::string rhs(s.top()); // V821 Decreased perfomance. The 'rhs' variable can be constructed in a lower level scope.
    s.pop();
    const std::string lhs(s.top());
    s.pop();

    if (fred(lhs))
    {
        v.push_back(rhs);
    }
    return v;
}

The warning from PVS studio is

V821 Decreased perfomance. The 'rhs' variable can be constructed in a lower level scope.

Since s is a std::stack-type, and the corresponding algorithm requires that the rhs-element is popped from the stack, it looks like PVS-Studio is wrong. Did i miss something?

By the way:

There is a typo in PVS Studio message:

   perfomance->performance

Reference

https://www.viva64.com/en/w/V821/

maybe PVS is talking about `rhs` begin only used if `fred(lhs)` is true. Even if you cannot change that (can you really not?) I would consider it as a true positive — 463035818_is_not_an_ai, Sep 05 '17 at 12:58
I'm not sure what sort of answer you hope to get from this. You could potentially rearrange the code slightly and it might change the warning, but in the end it probably doesn't matter. Even their documentation talks of false positives. https://www.viva64.com/en/w/V821/ — Retired Ninja, Sep 05 '17 at 13:04
You can std::move rhs and lhs because you pop them immediately after. It'll avoid the copy. — ratchet freak, Sep 05 '17 at 13:05
@tobi303: Thank you, but i do not see a way to change the order. — orbitcowboy, Sep 05 '17 at 13:24
@RetiredNinja: You are right. Their documentation talks about FPs. Thanks. — orbitcowboy, Sep 05 '17 at 13:26
You can still do `std::string rhs; std::swap(rhs, s.top());` as replacement of an actual move. — ratchet freak, Sep 05 '17 at 13:34

score 2 · Accepted Answer · answered Sep 06 '17 at 08:50

In comments ways of optimization of code are discussed. Yes, it can be optimized, though, I think there is practically no point in it. If you have to use C++-03, then, because of optimization, code will become complicated for understanding, which is bad. Well, sure, it would be appropriate to use std::move.

Now, speaking about PVS-Studio.The analyzer is not right, issuing a warning here. It is not possible to just take and relocate variable rhs creating inside the if-scope. The analyzer didn't tale it into account that the data source would change and s.top() would return the other value. Well, V821 diagnostic is new and there are shortcomings. We'll try to remove such a kind of false positives. Thank you for the given example, and also for the information about the typo the word "performance".

False positive warning PVS Studio: V821 Decreased perfomance. The 'rhs' variable can be constructed in a lower level scope

1 Answers1