What is the history behind performance warnings to copy parameters by const reference for Qt classes that use Copy On Write

[Steve Albright]

In general I really like using const reference for parameters but I am getting a lot of pushback by other developers saying it is a waste of time.

If we focus on QString for instance with uses Copy On Write (COW) why the performance warnings when a parameter is passed in by value?

Here is the information on Qt and implicit sharing: https://doc.qt.io/qt-5/implicit-sharing.html

A point of contention is that this should be considered a cheap copy: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#f15-prefer-simple-and-conventional-ways-of-passing-information therefore by value is fine.

Was that taken into consideration? Was too broad of a brush used for this rule, should we be concerned about internal implementation details, other?

Other concerns mentioned were that with copy by value you don't have to worry about threading issues and dangling pointers - something that std::string apparently failed on with their COW implementation.

Some other posts we looked into:
https://stackoverflow.com/questions/6792134/function-parameters-as-const-reference
https://stackoverflow.com/questions/12199710/legality-of-cow-stdstring-implementation-in-c11
https://stackoverflow.com/questions/11408698/qt-pass-by-const-reference-more-efficient-why?lq=1

I even wrote a test and it was faster and that isn't being debated but is it worth it and why is CPPCheck calling it out for QString (and other COW classes) is in question.

There are such strong feelings on this that we will might end up removing this performance warning (hopefully just for the COW classes) if it doesn't have good justification.

[Steve Albright]

@danielmarjamaki - any comments on this?

[Daniel Marjamäki]

I think it would be good to revisit this check. It's an old check. I did not consider QString and I agree it should be considered. Maybe it should recommend passing by value and using std::move in some situations.

[Steve Albright]

With the new return by reference check, I am getting more pushback on making changes for the same reason.

My preference is const references as much as possible and I don't understand this pushback.

Is CPPCheck going to revisit this or has the decision been made?

[Daniel Marjamäki]

Please feel free to adjust the checking.

I don't have time to look at all tasks I want. :-( So help on this would be appreciated.

I would assume there is also push back for parameters because sometimes it's preferable to use std::move instead of const references. I implemented the checker before there was std::move so it doesn't properly determine if std::move or const reference should be used.

Why there is push back for the new return by reference checking, I don't know. Except when there is some clever classes that does not copy all the data.

If we can avoid false positives for QString, etc that sounds good to me. Maybe we need some configuration option in the qt.cfg..

[Oliver Stöneberg]

Qt doesn't really follow the pattern of returning const references because of the way the objects are implemented.

Although the copy of QString does not imply the same cost as a non-Qt std::string it will still do the ref-counting stuff. that might be dismissible in most cases but in a very hot path that might still cause performance issues. Looking at the Qt documentation they seem to indicate it is mainly about memory usage. That used to be a major issue in earlier Qt versions possibly because people where just using copies and there was few tooling which pointed out that it was an unnecessary copy (people still regularly use auto instead of const auto& in range loops).

It is similar to std::shared_ptr and it often comes more down to the coding guidelines or personal flavor on how people handle this. In some cases it might just be that you are not aware of what is going on e.g. std::function which might also become heavy in case it is a lamdba with heavy copies in the capture.

I will see if I can put together a benchmark in the next few days.

[CHR]

Not sure if that is worth it. Since there is no downside at all to using const refs (except people not wishing to touch their code), I don't see why we should have extra code to accomodate copy-on-write classes.

[Steve Albright]

A possible downside could be that the content of your const reference changes (maybe multi-threaded - mentioned above too) whereas a copy does not.

Some of this is being discussed related to the return a reference check here: https://sourceforge.net/p/cppcheck/discussion/general/thread/184a00605c/

I use const references as much as possible for parameter types and haven't seen any problems, but I'm interested to see how this plays out with the (similar?) return a reference check.