swap(int&, int&) function isn't working, when swapped without using temp variable?

Question

I was writing the quickSort() function to sort an int[] when I discovered this -well, I don't what should I call it- undefined behaviour or error or some kinda thing that I can't understand right now.

quickSort program:

// quick sort
#include <iostream>
#include <cstdlib>
#include <ctime>
#include <iomanip>
using namespace std;

// constant
enum {MAX = 10};

// class
class A
{
private:
    int arr[MAX];
public:
    A() // constructor
    {
        srand(time(0));
        for(int i = 0; i < MAX; i++)
            arr[i] = rand() % 100;
    }
    // accessing array
    int get(int i)
    {
       return arr[i];
    }
    // quick sort
    int Partition(int left, int right);
    void quickSort(int left, int right);
    void Swap(int&, int&);
};

// member function definition
int A::Partition(int left, int right)
{
    int pivot = arr[right], i = left - 1;
    for(int j = left; j <= right-1; j++)
    {
        if(arr[j] < pivot)
        {
            i++;
            Swap(arr[i], arr[j]);
        }
    }
    Swap(arr[i+1], arr[right]);
    return i+1;
}

void A::quickSort(int left, int right)
{
    if(left < right)
    {
        int pi = Partition(left, right);
        quickSort(left, pi-1);
        quickSort(pi+1, right);
    }
}

void A::Swap(int& a, int& b)
{
    a = a+b;
    b = a-b;
    a = a-b;
}

// driver
int main(void)
{
    A obj1;

    //-------Array initialized--------
    cout << "Before sorting:" << endl;
    for(int i = 0; i < MAX; i++)
        cout << setw(4) << obj1.get(i);

    //--------Sorting Array-----------
    obj1.quickSort(0, MAX-1);

    //--------Sorted Array------------
    cout << "\nAfter sorting:" << endl;
    for(int i = 0; i < MAX; i++)
        cout << setw(4) << obj1.get(i);

    return 0;
}

The problem is inside the swap() function. When I am swapping the values using a int temp variable, values gets swapped and array gets sorted in ascending order.

But when I am swapping the values without using a temporary int variable, I'm getting 0s in the sorted array. As shown below:

Output:

Before sorting:
   89   43   18   98   23   88   52   18   1   25
After sorting:
   1   18   0   0   25   43   0   88   89   98

When I debugged the swap(), the parameters a and b were referring to the same address this.

If you want to enter the marks of **5** subjects, why did you define `marks` as an array of **4** ints? — Blaze, Oct 08 '19 at 06:58
You should enable compiler warnings too: `warning: format ‘%d’ expects argument of type ‘int *’, but argument 2 has type ‘char (*)[20]’ [-Wformat=] scanf("%d", &name);` — Mathieu, Oct 08 '19 at 07:02
As @DavidRanieri said, you are still writing outside the bound. ```marks[4]``` has 4 entries: ```marks[0]``` to ```marks[3]```! — agentsmith, Oct 08 '19 at 07:02
Please don't edit the issues out of the code in your OP as they are spotted. This is confusing to future readers. — moooeeeep, Oct 08 '19 at 07:06
This line scanf("%d", &name) is not correct. You say you want to read an int but then you specify char*. — Roya Ghasemzadeh, Oct 08 '19 at 07:23

Gor Asatryan · Answer 1 · 2019-10-08T08:32:04.907

2

You are using wrong format to get name of student. change scanf("%d", &name); to scanf("%s", name);

Also int marks[4] creates 4 elements. But in for loop you are reading 5 marks. When i=4 marks[i] is 5th element. So you have to change int marks[4] to int marks[5]

I think it will be better to define array size as constant value.

#define MARKS_COUNT 5

int main()
{
    // ...
    int total = 0, i, marks[MARKS_COUNT];
    // ...

    for(i=0; i<MARKS_COUNT; i++){
        scanf("%d", &marks[i]);
    }
    for(i=0; i<MARKS_COUNT; i++){
        total += marks[i];
    }

    avr=(float)total/MARKS_COUNT;
    // ...
}

Also read about format string here http://www.cplusplus.com/reference/cstdio/scanf/

edited Oct 08 '19 at 08:32

answered Oct 08 '19 at 07:04

Gor Asatryan

904
7
24

Well, now I'd say, it's better to `scanf("%20s", name)`. – Shubham Dec 11 '20 at 09:58
1

I think it is a better practice to use constexpr instead of macros. More info [here](https://stackoverflow.com/questions/42388077/constexpr-vs-macros). – Gary Strivin' Dec 16 '20 at 04:33

score 0 · Answer 2 · answered Oct 08 '19 at 08:23

why this is happening?

The array marks[4] is too small and assigning to marks[4] is invalid. In scanf("%d", &marks[i]); when i == 4 you are assigning a value to marks[4]. The standard says it's undefined behavior access marks[4] in any way, so from a standard point of view the program has undefined behavior and anything can happen.

I guess most probably your compiler has placed the local variables such that &marks[4] == &name[0] - the address of element one past the end of marks array is the same address of the name array.

Most probably on your system sizeof(int) >= 2. When scanf reads 90 from the input, it stores sizeof(int) bytes into the pointer you have given it. Assuming &marks[4] == &name[0], scanf("%d", &marks[4]) on execution becomes scanf("%d", name);.

Assuming your environment is sane CHAR_BIT = 8 and your environment is little endian, scanf really stores one byte 90 into the first byte of the pointer and clears the other bytes. So sscanf("90", "%d", &name[0]) would be "semi"-equal to writing sizeof(int) bytes into name - one with value 90 and sizeof(int) - 1 bytes with zeros, much like memcpy(&name[0], (char[sizeof(int)]){90,0}, sizeof(int)).

Strings in C are zero terminated and because the integer 90 is the letter Z in ascii and zero is the string termination character, when you later do:

printf("%s", name);

The name actually points to a null terminated string with the first byte 'Z' and the second byte being the zero terminating character, so it prints just Z.

swap(int&, int&) function isn't working, when swapped without using temp variable?

quickSort program:

Output:

2 Answers2