Different output every time in c++(unexpected behavior)

Question

this is the code for merge sort and sometimes it gives the right output but sometimes it gives output where one value is changed .

#include "bits/stdc++.h"
using namespace std;

//function to merge two array
vector<int> merging(vector<int> a,vector<int> b){   
    int x = (int)a.size() + (int)b.size();
    vector<int> v(x);
    int p = 0;
    int q = 0;
    for(int i=0;i<x;++i){
        if((q<(int)b.size())?a[p]<b[q]:true && p<(int)a.size()){
            v[i] = a[p];
            p++;
        }else{
            v[i] = b[q];
            q++;
        }
    } 
    return v;
}


//splitting the array and then merging the array
vector<int> mergeSort(vector<int> k){
   int x = (int)k.size();
   if(x<2){
       return k;
   }
   vector<int> a(k.begin(),k.begin()+(x/2));
   vector<int> b(k.begin()+(x/2),k.end());
   return merging(mergeSort(a),mergeSort(b));
}


int main(){
    vector<int> v = {3,5,34,11,32,7,35,54,67,89,23,4,3};
    //calling the merge function
    vector<int> b = mergeSort(v);
    for(int i=0;i<(int)b.size();++i){
        cout << b[i] << "\n";
    }
  return 0;
}

sometime output is expected 3 3 4 5 7 11 23 32 34 35 54 67 89

sometime output is 3 3 4 5 7 11 23 32 34 -423887504 35 54 67

Answer 1

Okay first off

if((q<(int)b.size())?a[p]<b[q]:true && p<(int)a.size()){

Yikes.

That's some truly convoluted logic. You've got a ternary inside an if, you have true && ... (which is the same thing as just ... there) and there's absolutely no spacing anywhere, making the whole thing even further unreadable. This would not pass any code review that occured within 100 feet of me. Even before I looked too far into your code, I guessed this was where the problem was.

So in your example, when you try to merge one final time you'll have the following values:

a = {35, 54, 67}
b = {3, 4, 23, 89}

Let's walk through it with your merge... All is well for the first few loops until you have:

p = 3
q = 3
x = 7
i = 6
v = {3, 4, 23, 35, 54, 67, 0}

Now, we're heading into the loop, i < x is true, so we're still going. And we get to your interesting if.

q < b.size() is true, 3 < 4. So we look at a[p] < b[q] or a[3] < b[3]. Uh oh! Do you see the problem? a[3] is out of range. Which means undefined behavior.

Instead of trying to do it all in one loop, I'd try to write clean code instead of short code and user another loop. This loop will empty out whichever of a or b you have left after you empty the other (or the way I've written it, it'll be two loops, only one of which will be run).

That'd look like:

size_t p = 0, q = 0, i = 0;

while(p < a.size() && q < b.size()) {
    if(a[p] < b[q]) {
        v[i++] = a[p++];
    } else {
        v[i++] = b[q++];
    }
}

while(p < a.size()) {
    v[i++] = a[p++];
}

while(q < b.size()) {
    v[i++] = b[q++];
}

You'll notice a couple things I've changed here:

• I've moved from int to size_t so you can avoid all of those ugly casts everywhere since your variables will already match the type of the vector's sizes.
• I've split off two loops to empty whichever of the two vectors still has values in it.
• I've moved to while's, as I think they look nicer here.

Answer 2

Few things to notice:

Operator percedence - ((q < b.size()) ? a[p] < b[q] : true && p < a.size()) is equivalent to: (q < b.size() ? a[p] < b[q] : p < a.size()). Note that you don't consider p, which, unfortunately might be over the end (undefined behaviour) - Note that this is your bug. The fix is rather simple -

(q < b.size() && p < a.size() ? a[p] < b[q] : p < a.size()) 

Which applies the comparison only if there are any elements left on both the arrays.

Moreover, there are many bad patterns in your code:

First of all, you have a lot of redundant C style casts. This is bad for two reasons: 1. You are writing C++, use C++ casts! (static_cast in your case) 2. You are casting for no apparent reason. There is a very good reason the type of size in vectors (and generally in C++) is std::size_t. Use it!

Another thing in your code is that you pass arguments as value and not as const-ref which is a very bad practice and causes unnecessary copies happening all around.

Third, your variable names are meaningless.

And fourth you use the namespace std which is highly not recommended.

Answer 3

May be the trouble in this line:

if((q<(int)b.size())?a[p]<b[q]:true && p<(int)a.size()){

you don't check the size of array 'a' before using a[p]. So, sometimes (due to waste in memory), a[p] can be less than b[q], sometimes no.

As a variant, of course. Lets check array size before using.