std::swap(); not working when used in an else statement

Question

Here is a set of conditionals which moves a char in a vector, using the swap function, depending on certain variables:

 if (m == 's')
{
  if (currenti == 5)
  {
    swap(height[currenti][currentj], height[0][currentj]);
  }
  else if (height[currenti++][currentj] == 'T')
  {
    cout << "Game over!" << endl;
    gameLive = 0;
  }
  else
  {
    swap(height[currenti][currentj], height[currenti++][currentj]);
  }
}

For some reason the swap in the else part doesn't swap the two vector parts, even though it is called. (I put in a cout, in the else section, to check this). However, if I remove the other conditionals like this:

if (m == 's')
{
  swap(height[currenti][currentj], height[currenti++][currentj]);
}

It works completely fine.

Does anyone know what is causing it to not work in the "else" format?

Odds are the `currenti++` is taking effect AFTER the function runs. Best not to take risks like this. — user4581301, Oct 26 '19 at 15:31
`else if (height[currenti++][currentj] == 'T')` will increment `currenti`, regardless whether you enter the `if()`. — Fureeish, Oct 26 '19 at 15:31
Possible duplicate of [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) — Max Vollmer, Oct 26 '19 at 15:32

jignatius · Answer 1 · 2019-10-26T16:08:36.060

1

swap(height[currenti][currentj], height[currenti++][currentj]);

This is swapping the same value. current++ is post-increment, so its value is only incremented after the line.

I think you mean to do this:

swap(height[currenti][currentj], height[currenti+1][currentj]);

You probably do want to increment currenti after the line above. You could use pre-increment for that:

++currenti;

There's a slight performance benefit with pre-increment since the value is incremented first, but with post-increment a copy of the original value is used.

edited Oct 26 '19 at 16:08

answered Oct 26 '19 at 15:36

jignatius

6,304
2
15
30

Two points: 1. You don't necessarily know which parameter to `swap` will be evaluated first, so it's possible this could provide the expected results because the parameters are effectively backwards. 2. That `currenti++` probably still has to happen somewhere. I'd drop it in on the line after the `swap` – user4581301 Oct 26 '19 at 15:50
@user4581301 I agree that `currenti++` probably needs to happen after the line. However, I leave that decision to the OP. – jignatius Oct 26 '19 at 16:01

Adrian Mole · Answer 2 · 2019-10-26T16:07:25.547

1

This line:

swap(height[currenti][currentj], height[currenti++][currentj]); // Don't do it!!

Is swapping a value with itself! The expression height[currenti++][current] takes current value of currenti as the first index, then increments it.

You should use code like this for your else block:

else
{
    swap(height[currenti][currentj], height[currenti+1][currentj]);
    ++currenti;
}

Note: It is tempting to use the pre-increment operator for the index in the second argument:

swap(height[currenti][currentj], height[++currenti][currentj]); // Don't do it!!

but this is undefined behaviour, as the order of evaluation of the arguments to std::swap is unspecified. One or other of the two (post- or pre-increment) may work, on some platforms, some of the time, but you can never rely on it.

edited Oct 26 '19 at 16:07

answered Oct 26 '19 at 15:37

Adrian Mole

49,934
160
51
83

Your closing statement slightly derails the opening statement. Since the order is unspecified (undefined before C++17), it's possible the second parameter is resolved first and `swap(height[currenti][currentj], height[currenti++][currentj]);` behaves as expected. – user4581301 Oct 26 '19 at 15:43
@user4581301 - See edit! Hope this explains the 'slight' derailment issue. – Adrian Mole Oct 26 '19 at 15:45
@user4581301 OK, I now get what you mean! Either way, the recommended code will work safely, in whichever order the arguments are evaluated. – Adrian Mole Oct 26 '19 at 15:49
I thought it *was* undefined behavior to use an auto-increment on a variable that is used in other places in the statement. Did this change? – Mark Ransom Oct 26 '19 at 15:55
@MarkRansom I think you are correct! I have edit my answer to reflect this. Thanks. – Adrian Mole Oct 26 '19 at 16:08
1

cppreference says `f(++i, ++i); // undefined behavior until C++17, unspecified after C++17`, but it also says *In a function call, value computations and side effects of the initialization of every parameter are indeterminately sequenced with respect to value computations and side effects of any other parameter.* So I think the order of the evaluation is still undefined, but you're guaranteed that each evaluation is complete before proceeding, no matter what the order. – user4581301 Oct 26 '19 at 16:45

std::swap(); not working when used in an else statement

2 Answers2