2

Swap with temp variable is not giving any output but swap() is giving an desired output. I am trying to swap arr[i] with arr[arr[i]+1].

This code is giving the desired output

#include <iostream>

using namespace std;

int main()
{
    int n;
    cin>>n;
    int arr[n],i,temp;
    for(i=0;i<n;i++) {
        cin>>arr[i];
    }
    for(i=0;i<n;i++) {
        swap(arr[i],arr[arr[i]+1]);
    }
    for(i=0;i<n;i++) {
        cout<<arr[i]<<" ";
    }
}

But with temp variable there's no output. Code is given below

#include <iostream>

using namespace std;

int main()
{
    int n;
    cin>>n;
    int arr[n],i,temp;
    for(i=0;i<n;i++) {
        cin>>arr[i];
    }
    for(i=0;i<n;i++) {
        temp=arr[i];
        arr[i]=arr[arr[i]+1];
        arr[arr[i]+1]=temp;
    }
    for(i=0;i<n;i++) {
        cout<<arr[i]<<" ";
    }
}

I don't know what's the issue. I hope will get an response soon. Thank You!

Mayank Kumar Sohanda
  • 23
  • 3
  • 1
    `arr[arr[i]+1]` is this really what you mean? What happens when `i == n - 1`? – Passerby Jan 15 '22 at 01:50
  • 1
    Depending on what values are in `arr`, the code `arr[arr[i]+1]` may access values out of bounds of `arr`. For example, if `arr[i] + 1` is less than zero or more than `n-1`. The behaviour is then undefined. One consequence of undefined behaviour is that two otherwise equivalent sets of operations (e.g. swapping in different ways) can produce different outcomes. Also `int arr[n]` where `n` is a variable is not valid C++ (it is a non-standard extension supported by SOME compilers). – Peter Jan 15 '22 at 02:22
  • 2
    [VLAs are not a thing in C++](https://stackoverflow.com/q/39334435/430766). – bitmask Jan 15 '22 at 02:41
  • 1
    `int n...int arr[n]` -- This is not valid C++. If you are learning C++ by going to certain websites, don't go to those websites. Good C++ books never show declaring arrays this way, where the size is declared with a runtime variable. The reason why they don't show this is because it isn't C++. Instead, use `std::vector arr(n);` -- That is the proper, standard, C++ way to declare a dynamic array. – PaulMcKenzie Jan 15 '22 at 02:48
  • Also, you failed to include the proper header for [std::swap](https://en.cppreference.com/w/cpp/algorithm/swap). Also, and especially for functions called `swap`, do not have `using namespace std;`. There have been issues with programmers having their own `swap` function, and the compiler confusing it for the `std::swap` function, all because `using namespace std;` appears in the file. – PaulMcKenzie Jan 15 '22 at 02:57
  • 2
    In your manual `temp` swap, the line `arr[arr[i]+1]=temp;` is using the new value of `arr[i]`, but the `swap` version uses the old value of `arr[i]`. – Raymond Chen Jan 15 '22 at 04:17

3 Answers3

6

The different results is because std::swap does not use a temporary variable this way, it works slightly differently. Hence the different results.

To understand the different results consider the most simple case when the array has two values:

      +---+---+
arr:  | 0 | 1 |
      +---+---+

arr[0] is 0 and arr[1] is 1. This can't be any more simpler, but let's just consider the only the initial swap, when i is 0:

std::swap(arr[i],arr[arr[i]+1]);

Let's evaluate both parameters when i is 0.

The first parameter to std::swap is, therefore arr[0].

The second parameter to std::swap is arr[arr[0]+1]. arr[0] is 0, so this becomes arr[0+1], or arr[1].

Conclusion: the first parameter to std::swap is arr[0], the second parameter is arr[1].

std::swap then proceeds and swaps arr[0] with arr[1]. The exact way it does this is not important, it's only important to understand that the two values get swapped as expected.

Now, let's walk through what happens, with the same arr, when a temporary variable is used. Let's do this, one step at a time:

        temp=arr[i];

Since i is 0, this becomes temp=arr[0], so temp becomes 0.

        arr[i]=arr[arr[i]+1];

i is still 0, so this becomes arr[0]=arr[arr[0]+1]. arr[0] is still 0, so this becomes arr[0]=arr[0+1], or arr[0]=arr[1].

The current contents of the array are now, at this point:

      +---+---+
arr:  | 1 | 1 |
      +---+---+

And now, the final statement gets executed:

        arr[arr[i]+1]=temp;

i is still 0. So, this becomes arr[arr[0]+1]=temp.

What it arr[0]? It's value is 1. So this becomes arr[1+1]=temp, or arr[2]=temp.

There is no arr[2]. Instead of swapping anything, the end result of the shown code is undefined behavior, memory corruption, and demons flying out of your nose.

Now, it's also possible to have undefined behavior using the std::swap version as well, depending on what's actually in the arr. This is only an explanation of why using a temporary variable is not the same as using std::swap, with all other things being equal.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • 1
    "it's also possible to have undefined behavior using the `std::swap` version" is incorrect. Both samples of OPs code (whether using `std::swap()` or not) give undefined behaviour. Undefined behaviour means that the outcome is not defined by the standard. It doesn't mean that the program is required to give a result that the programmer (or anyone else) considers strange or erroneous. – Peter Jan 15 '22 at 02:28
1

Bug is in the below lines of code

temp=arr[i];
arr[i]=arr[arr[i]+1];
arr[arr[i]+1]=temp;

Why there is bug :

suppose arr[0] is 0 and arr[1]=1
temp=arr[0]// now temp is 0
arr[i]=arr[arr[i]+1];// now arr[0] is 1 
arr[arr[i]+1]=temp;//arr[2] is now 0 and about arr[1] we are not setting it and arr[2] value is also lost

What is the solution :

Store the values before swapping :

int temp1=arr2[i];
int temp2=arr2[arr2[i]+1];
//swapping
arr2[arr2[i]+1]=temp1;
arr2[i]=temp2;//it should be below, don't assign arr2[i] earlier

Source with logical bug fixed :

#include <iostream>

using namespace std;

//swap
void swap(int &a,int &b){
    //cout<<"swap "<<a<<" "<<b<<endl;
    int temp=b;
    b=a;
    a=temp;
}

int main(){
    int n;
    cin>>n;
    //arr is swapped using swapped method
    int arr[n],i,temp;
    //arr2 is swapped using temp variable
    int arr2[n];
    for(i=0;i<n;i++) {
        cin>>arr[i];
        arr2[i]=arr[i];
    }
    for(i=0;i<n;i++) {
        swap(arr[i],arr[arr[i]+1]);
        //cout<<"swap2 "<<arr2[i]<<" "<<arr2[arr2[i]+1]<<endl;
        //swap(arr2[i],arr2[arr2[i]+1]);
        //store both the varibles before giving value
        int temp1=arr2[i];
        int temp2=arr2[arr2[i]+1];
        //arr2[i]=arr2[arr2[i]+1];
        //arr2[arr2[i]+1]=temp;
        //swapping
        arr2[arr2[i]+1]=temp1;
        arr2[i]=temp2;
        /*
           cout<<"after swapping arr"<<endl;
           for(int j=0;j<n;j++) {
           cout<<arr[j]<<" ";
           }
           cout<<endl;
           cout<<"after swapping arr2"<<endl;
           for(int j=0;j<n;j++) {
           cout<<arr2[j]<<" ";
           }
           cout<<endl;
           */
    }
    for(i=0;i<n;i++) {
        cout<<arr[i]<<" ";
    }
    cout<<endl;
    for(i=0;i<n;i++) {
        cout<<arr2[i]<<" ";
    }
}

Input :

$ g++ array.cpp && ./a.out
4
0 1 2 2
1 0 2 2 
1 0 2 2
Udesh
  • 2,415
  • 2
  • 22
  • 32
-1

If your goal is to swap every element with the element next to it as you move through the array from 0 to n then all you need is: swap(arr[i], arr[i+1]); You will have to be sure to stop before i+1 is greater than the number of indexes in your array.

if(i+1 >= n) break;

janst
  • 102
  • 10