1526823 - const by-value parameters can be pessimizations

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

In https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/ , Arthur O'Dwyer argues against const by-value parameters.
IIUC, the rationale is that the function signature is a contract between the caller and the function, but in the case of a const by-value parameter, const is more an internal detail of the function implementation.
Though this rationale could be thought subjective, I think the most important point is that in many cases, a const by-value parameter may in fact be a typo (missing &), or will prevent more optimal code:

const on a by-value parameter can be a pessimization

I looked for this kind of typo in my current employer’s codebase — code written by professional programmers with years of C++ experience. I found an absolutely astounding number of them! The most common and egregious variation in our codebase looks like this:

class Widget {
    std::string name_;
public:
    void set_name(const std::string name) { name_ = name; }
};

This function will pass all the unit tests you throw at it, but it makes an additional heap-allocation compared to either
void set_name(const std::string& name) { name_ = name; }
or
void set_name(std::string name) { name_ = std::move(name); }

Here's a corresponding (imperfect) searchfox grep:
https://searchfox.org/mozilla-central/search?q=%5B(%2C%5Dconst+%5BA-Za-z0-9_%3A%5D%2B+%5BA-Za-z0-9_%3A%5D%2B%5B%2C)%5D&case=false&regexp=true&path=

Lots of instances look harmless (mostly POD types), but there may be some real pessimizations, e.g., here's one with const std::string:
https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/ipc/glue/GeckoChildProcessHost.cpp#1080

I admit I'm not too sure whether we should be so bold as to prevent any const by-value parameters, but it may be worth exploring some ways to find real issues.
Maybe as a start a static checker could focus on const non-POD types?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:sylvestre, could you have a look please?