6

I ran clang-tidy (the "modernize" modules) on a project tree that I have been trying to keep reasonably up to date with C++17. Almost everything it corrected was unsurprising to me, except for one thing: It changed all of these types of constructs:

void foo(const std::string& str) {
}

.. to this:

void foo(std::string str) {
}

And I don't understand why. To my untrained eye this would mean two things:

  1. It would need to copy the object rather than just pass a reference. (Though I assume there are situations when the compiler can deduce that it can just pass a pointer when it generates the code -- but the reference makes it explicit (which is better imho)).
  2. The const is there to tell the function body developer that it shouldn't be changing the input string, and that if it needs to modify the string it needs to store its own copy somewhere.

I do see an upside though -- by just passing an object as a const reference it's a mere "remove const" cast away from being changed anyway, so I guess passing by value would solve that.

Why does it recommend removing const references with non-const pass-by-values?

1
  • 2
    In your case I would take a std::string_view instead of a constant reference. To me a std::string_view replaces the need for constant string references
    – WBuck
    Apr 10, 2020 at 21:59

1 Answer 1

8

The rationale given here is

With move semantics added to the language and the standard library updated with move constructors added for many types it is now interesting to take an argument directly by value, instead of by const-reference, and then copy. This check allows the compiler to take care of choosing the best way to construct the copy.

Additionally

The transformation is usually beneficial when the calling code passes an rvalue and assumes the move construction is a cheap operation.

However, the documentation states that the only replacement is in the following specific case:

Replaces the uses of const-references constructor parameters that are copied into class fields. The parameter is then moved with std::move().

It doesn't even apply the transformation if the constructor parameter is used more than once.

So I don't think all your functions should have been transformed like that.

4
  • You are right -- it hasn't transformed all of them; just so many that I thought it was all of them. Apr 10, 2020 at 20:29
  • 1
    Just a heads up, clang-tidy is great, but you should always double check the transformations to make sure they don't break anything. The best way is to have lots and lots of test :)
    – cigien
    Apr 10, 2020 at 20:31
  • Yupp -- I ran a bunch of modernize modules, ran all the unit tests, function tests and system tests and they all passed. I was genuinely pleasantly surprised. That said, I have still not dared commit anything yet. And the tests were run on only one platform so far. Apr 10, 2020 at 20:37
  • 1
    Ha, I know the feeling. But if you've got a bunch of tests, that's great; be bold, and go for it :)
    – cigien
    Apr 10, 2020 at 20:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.