7

I have a function which looks like this 

int myclass::setVersion(std::string ver)
{
  if (ver.size()>1)
  {
    version.swap(ver)
    return 0;
  }
  else 
    return -1;
}

My question is very simple, is it better to pass ver the way it is or better to pass it as a pointer to string? FYI, ver is a small size string (around 8). EDIT: it does not matter if ver is changed. I just want to replace version with the contents of ver. EDIT2: I am using visual studio 2008.

Samer
  • 1,923
  • 3
  • 34
  • 54
  • 2
    It's not a question of "better". If the function is supposed to modify its argument, then it has to be passed by reference (or pointer, if you insist). If it's not, then it has to be passed by value. – Mike Seymour Oct 14 '14 at 14:24
  • In your original question your title mentioned arrays, but there were no arrays in the question. – Some programmer dude Oct 14 '14 at 14:33

5 Answers5

6

It depends on what you want the behavior of the code to be.

Most of the suggested answers here change the behavior from your posted code in that they will modify the string that's passed in (or make the code fail to compile because it can't modify the passed in argument).

In the example you posted,the string passed to myclass::setVersion() will not be modified (the parameter may be modified, but that is just a copy fo the string passed in; a copy which will be destroyed when the function returns).

For a a case like this, I'd suggest passing a const std::string&:

int myclass::setVersion(std::string const& ver)
{
  if (ver.size()>1)
  {
    version = ver;
    return 0;
  }
  else 
    return -1;
}

This way the copy is made only when necessary.

But honestly, unless the function is called often, it's probably nothing to worry much about.

Michael Burr
  • 333,147
  • 50
  • 533
  • 760
  • ***But honestly, unless the function is called often, it's probably nothing to worry much about.*** That is exactly my thoughts. I mean I assume the version string will not be a KB or not be called a million times so the impact of copying would be almost zero. – drescherjm Oct 14 '14 at 14:51
  • 1
    Start with this answer. If measurements show you need to optimize the case where you are passing an rvalue then add an rvalue-ref overload. – Chris Drew Oct 14 '14 at 15:14
  • 1
    My rule of thumb is to use `const&` by 'default'. If the function would always end up copying the parameter in order to make a modifiable copy, then instead the argument changes to a pass-by-value. Other details (like the caller's argument needs to be modified, or to better support rvalue arguments with C++11 move semantics) will result in those details changing or an overload to cover the additional requirements. – Michael Burr Oct 14 '14 at 16:10
5

In C++11 you can add an overload which accepts rvalue reference:

int myclass::setVersion(std::string& ver);
int myclass::setVersion(std::string&& ver);

This way you'll be able to swap from rvalues as well as lvalues. And actually instead of swap you can perform a move assignment:

version = std::move(ver);

This is potentially faster than swap, depending on std::string implementation.

Examples of usage:

string getVersion() { return "version2"; }

string v1 = "version1";
a.setVersion(v1);           // lvalue
a.setVersion(getVersion()); // rvalue
a.setVersion("version3");   // rvalue as well

Demo

UPDATE: Indeed, you have to have both variants of setVersion for maximum flexibility. But I agree with others in that all this is premature optimization and should be used only if profiling shows a bottleneck at this point.

Anton Savin
  • 40,838
  • 8
  • 54
  • 90
  • What is the advantage of this approach to passing by const reference and assigning the string? – Neil Kirk Oct 14 '14 at 14:36
  • 1
    @NeilKirk swapping is potentially faster than copying. Actually `setVersion` can be implemented even faster than swap with move semantics. – Anton Savin Oct 14 '14 at 14:38
  • 1
    Note that with this suggestion you'll need an overload that takes a standard reference if you want to be able to pass in `v1` without moving it (using `std::move()`) and therefore making its value indeterminate. – Michael Burr Oct 14 '14 at 14:46
  • Writing a function which takes an rvalue is usually a premature optimization and can even result in slower and less safe code due to the need to make copies to obtain those precious rvalues. Just because a new feature exists doesn't mean you should use it everywhere and as @MichaelBurr says `const&` should still be your default. – sjdowling Oct 14 '14 at 16:31
  • @MichaelBurr thanks, code & demo fixed, footnote added. – Anton Savin Oct 14 '14 at 17:49
3

Please pass a reference.

int myclass::setVersion(std::string& ver)
elimad
  • 1,132
  • 1
  • 15
  • 27
3

You want to store the contents of ver into version. A copy has to be made, the question is where.

You can do it the way you propose, as swapping is fast. But I notice not all control paths cause the version string to be assigned a new value.

Therefore I suggest the following

int myclass::setVersion(const std::string& ver)
{
  if (ver.size()>1)
  {
    version = ver;
    return 0;
  }
  else 
    return -1;
}

This will avoid a copy should the size of ver be 1, in which case you don't want to copy the string into version.

Neil Kirk
  • 21,327
  • 9
  • 53
  • 91
2

You should pass it in by reference whenever you are modifying the string and wanting the caller to see that change.

Laura Maftei
  • 1,863
  • 1
  • 15
  • 25