Safe default: passing string by value or by const ref?

[Václav Brožek]

Hi all,

There are (at least) 2 ways of writing a string setter in C++11:

void SetString(const std::string& str) {

  mystr = str;

}

vs.

void SetString(std::string str) {

  mystr = std::move(str);

}

They both are reasonably simple: the first one bothers with a longer argument type, the second with the explicit move.

The first always does one string copy (during the assignment), the latter does sometimes no, sometimes one copy (depends on whether the argument is a temporary or not).

Is it acceptable to use the second version in Chromium as a safe default?

(We hit a concrete case of this in a code review.)

Cheers,

Vaclav

[Bence Béky]

Hi,

My understanding is that the first option is preferred in general,
because no allocation is needed if |str| fits in |mystr| member's
existing capacity. See Herb Sutter's talk [1] and slides around page
27 [2], including actual benchmarks with a number of compilers.

That being said, there might exist contradicting Google or
Chromium-specific guidelines that I am not aware of.

Cheers,

Bence

[1] https://www.youtube.com/watch?v=xnqTKD8uD64&feature=youtu.be&t=1h4m28s
[2] https://github.com/CppCon/CppCon2014/blob/master/Presentations/Back%20to%20the%20Basics!%20Essentials%20of%20Modern%20C%2B%2B%20Style/Back%20to%20the%20Basics!%20Essentials%20of%20Modern%20C%2B%2B%20Style%20-%20Herb%20Sutter%20-%20CppCon%202014.pdf

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

We prefer the first option.

(Previous discussion: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/string$20const$20ref/chromium-dev/BCONSC5ciBE/ri16CxvJBAAJ)

[Joe Mason]

I worry that contributors who aren't familiar with the idiom would copy method signatures that don't have the const-ref, but not know to copy the move in the implementation. Preventing that is a bit of extra work for reviewers.

The back-and-forth in your bug nicely avoids that, BTW - the reviewer reviewer challenged why you didn't have a const-ref and you were able to explain. It would have been a red flag if you'd responded, "I just did it that way because SetFooString did that. Isn't that the normal style?" If reviewers are no longer specifically looking for "const string&" they would never have asked, and might have not noticed if the implementation was wrong.

So I prefer to keep "const string&" as the default, because it's easier to review since all the information is in the signature, not split between the signature and implementation. And because of that I would avoid std::move unless an individual use is shown to be a win for performance, even if it is safe as a default, because having two ways to do the same thing is extra load on reviewers.

On Mon, Jan 25, 2016 at 5:43 AM, Václav Brožek <va...@chromium.org> wrote:

--

[Václav Brožek]

Thanks for the answers, and the link to the older discussion (sorry for having missed that).

I understand the arguments, will change the value+move instance I recently introduced, and will stop suggesting value+move in reviews unless it is measurably improving performance in the given case.

Cheers,

Vaclav