fixed #12384 - removed misleading suggestion from passedByValue and iterateByValue messages

[firewave]

No description provided.

[chrchr-github]

Just stating the fact is probably not enough to steer users in the right direction. And if 'pass by value + move' is used, the object will still be passed by value.
On the other hand, a full discussion of the topic is too long for an (extended) error message. So maybe we should just mention the alternatives ('const ref', 'pass by value + move', 'universal ref'?).

[firewave]

Just stating the fact is probably not enough to steer users in the right direction. And if 'pass by value + move' is used, the object will still be passed by value. On the other hand, a full discussion of the topic is too long for an (extended) error message. So maybe we should just mention the alternatives ('const ref', 'pass by value + move', 'universal ref'?).

Putting a short summary of the solutions in the verbose message would make sense.

I have some changes which already adjusted some way too verbose messages. So the --errorlist output might deserve some review as a whole.

[firewave]

There is just too many possible solutions to include it at all. So no verbose message at all would be better.

Maybe we can just reword the message by including the term "unnecessary copy" which would also be closer to the performance-unnecessary-value-param clang-tidy message.

[danmar]

thanks for clarifying this message it's needed. 👍

Maybe we can just reword the message by including the term "unnecessary copy" which would also be closer to the performance-unnecessary-value-param clang-tidy message.

Sounds good.

So maybe we should just mention the alternatives ('const ref', 'pass by value + move', 'universal ref'?).

stupid question; what do you mean with 'universal ref'?

There is just too many possible solutions to include it at all. So no verbose message at all would

sorry are there that many possible solutions?

how about this verbose message:

the const qualified parameter 'x' is passed by value, leading to unnecessary copies.
Possible solutions:
 * make it a const-reference
 * make it non-const and always use std::move when function is called
 * universal ref..

The option "make it non-const and always use std::move when function is called" feels less generic to me. It can work in specific cases but in general if the function is designed to be for arbitrary usage then const reference is still preferable right?

[firewave]

sorry are there that many possible solutions?

on top of the mentioned ones:

• rvalues
• forwarding
• perfect forwarding
• explicitly moving without involving std::move()
• possible modern C++ trickery I do not care for

Any additional information will be just misleading and I would rather just have the most informative message about what was detected. The compilers/analyzers are also only provide fixits if there is only a single solution.

[danmar]

Possible solutions:

Maybe we can reword that somehow so it indicates that it's not a exhaustive list of all possible solutions. But only some suggestions how it can be solved. I think those suggestions can help the user to choose the optimal solution. Personally I feel that your other suggestions are a bit related to the std::move suggestion.

Any additional information will be just misleading and I would rather just have the most informative message about what was detected. The compilers/analyzers are also only provide fixits if there is only a single solution.

I would think that we can make the output more specific. A proper fixit is helpful. For this code:

void f(const std::string value) {
  sz = value.size();
}

Since "value" is not copied in this function, as far as I know it makes sense to suggest that "value" is made a const reference in this code. And clang-tidy also suggests making it a reference.

For a setter method that copy the parameter somehow, it might be a better idea to move.. it's unfortunate to explicitly recommend making it a const reference as we do now.. for information clang-tidy still suggests to make the parameter a const reference.

[chrchr-github]

So maybe we should just mention the alternatives ('const ref', 'pass by value + move', 'universal ref'?).

stupid question; what do you mean with 'universal ref'?

Basically passing an auto&& parameter. Apparently it's also called a forwarding reference:
https://stackoverflow.com/questions/39552272/is-there-a-difference-between-universal-references-and-forwarding-references

[firewave]

That would need additional logic to determine what the suggestion might be and that might not be 100%.

Another problem with making any suggestions is that beside the implementation you also need to take all callers into consideration.

Example:
If you change a copy into a const reference you also need to adjust all callers. That means you might need to drop std::move() calls along the way and that would change the lifetime of the objects. In that case the fix might actually be to make sure that the copy is consumed in the function body.

The problem is also with clang-tidy: llvm/llvm-project#57908.

Then there is the lack of detection of unnecessary copies/missing std::move() in the common tools (beside Coverity which also does not detect many of such cases) which would also be reported with such code: llvm/llvm-project#53489 / https://trac.cppcheck.net/ticket/8945.

And more related stuff:
https://trac.cppcheck.net/ticket/12384
https://trac.cppcheck.net/ticket/12627

[firewave]

Since we started documenting the checkers I think any suggestions on how to address this should be put into that.