0

I am writing code for bottom-up merge sort in c++, but this error occurs on a line where I am allocating memory to a pointer,

void merge(int *a, int low, int high, int mid,int max)
{
   int i, j, k; 
   int *c=new int[max];   //error occurs here
   i = low;
   k = low;
   j = mid + 1;
   while (i <= mid && j <= high)
   {
       if (a[i] < a[j])
       {
           c[k] = a[i];
           k++;
           i++;
       }
       else
       {
           c[k] = a[j];
           k++;
           j++;

       }
   }
   while (i <= mid)
   {
       c[k] = a[i];

       k++;
       i++;
   }
   while (j <= high)
   {
       c[k] = a[j];
       k++;
       j++;
   }
   for (i = low; i < k; i++)
   {
       a[i] = c[i];
   }
}

error screen shot

What is the problem, looks like no more memory available??

  • what's the value of max? – billz Mar 12 '14 at 08:08
  • it is 63, when error occurs – user3324329 Mar 12 '14 at 08:09
  • Are you calling it in a loop? – billz Mar 12 '14 at 08:09
  • Please don't post images. Put the code and textual error message directly into your post. Thank you. – poljpocket Mar 12 '14 at 08:10
  • @billz no, its not in loop, I have updated my question. – user3324329 Mar 12 '14 at 08:11
  • 1
    I don't suppose you're interested in `delete[]`-ing that array when you're done with it? And you have two more parameters to this function than you need if this is the merge-side of a merge-sort. All you need is a base, a mid, and a high. The other params aren't needed if the pointer arithmetic is done correctly on the caller-side. – WhozCraig Mar 12 '14 at 08:24
  • @WhozCraig delete isn't needed, when the function will return all the memory allocated will automatically be deleted. And in merge we do need the array of data, base, mid and high. I am using only one extra parameter and that could not cause error. – user3324329 Mar 12 '14 at 08:32
  • "when the function will return all the memory allocated will automatically be deleted" - You could not possibly be further from the truth with that statement. Your code as-written has a blatant memory leak. And no, you do *not* need a base offset. The pointer `a` *is* the base offset if called correctly. In truth, an ideal merge sort doesn't even need `mid`, since it is always `len/2` (but its usually provided for completeness). If you don't believe me I can show you. – WhozCraig Mar 12 '14 at 08:34
  • @user3324329 actually when the function return only the memory used my the pointer will be returned to the program; but he pointer is on the stack. The memory allocated on the heap will remain allocated - memory leak. – Pandrei Mar 12 '14 at 08:36
  • @WhozCraig its ok, i know it – user3324329 Mar 12 '14 at 08:44

1 Answers1

1

Your corruption problem is this:

k = low;

You're using k as the index into your temporary array. It should start at 0, not low Change it to:

k = 0;

Regarding the rest of your algorithm, you have a blatant memory leak. You're not deleting your temporary array. Also, the algorithm of merge sort is considerably easier in C and C++ than most languages precisely because of its native ability with pointer arithmetic

For example:

template<typename T>
void merge(T* ar, size_t mid, size_t len)
{
    T *merged = new T[len]; // note. normally i would use a smart pointer

    size_t i=0, j=mid, k=0;
    while (i<mid && j<len)
    {
        if (ar[i] < ar[j])
            merged[k++] = ar[i++];
        else
            merged[k++] = ar[j++];
    }

    while (i < mid)
        merged[k++] = ar[i++];
    while (j < len)
        merged[k++] = ar[j++];

    std::copy(merged, merged+k, ar);
    delete [] merged;
}

template<typename T>
void mergesort(T* ar, size_t len)
{
    if (len<2)
        return;

    mergesort(ar, len/2);
    mergesort(ar+len/2, len-len/2);
    merge(ar, len/2, len);
}

Using the above. the following demonstrates how it is called:

int main()
{
    std::random_device rd;
    std::default_random_engine rng(rd());
    std::uniform_int_distribution<> dist(1,99);

    std::vector<int> data;
    data.reserve(25);
    std::generate_n(std::back_inserter(data), data.capacity(),
                    [&](){ return dist(rng);});

    for (auto x : data)
        std::cout << x << ' ';
    std::cout << '\n';

    mergesort(data.data(), data.size());

    for (auto x : data)
        std::cout << x << ' ';
    std::cout << '\n';

    return 0;
}

Output (obviously varies)

55 39 40 87 49 1 94 8 20 47 42 23 93 99 81 52 17 66 3 6 74 5 49 13 67 
1 3 5 6 8 13 17 20 23 39 40 42 47 49 49 52 55 66 67 74 81 87 93 94 99 

All of that said, unless this is for academia you're crazy not to just use std::sort


Alternative Allocation

The bottom-up design works well in different cases, but the memory allocation system it utilizes is dreadful for large sets. The total temp space needed will never be more than N items for an N-item container, so you can allocated it up-front with a front-loader to the actual merge sort that is then, along with the merge algorithm, provided temp space to use rather than allocating their own. The only tricky part is knowing where in the tmp storage it is safe to use space. But you already know that. The same offsets you're using for your sequence being sorted can also be used for the proper matching temp-space locations:

template<typename T>
void merge(T* ar, size_t mid, size_t len, T* tmp)
{
    size_t i=0, j=mid, k=0;
    while (i<mid && j<len)
    {
        if (ar[i] < ar[j])
            tmp[k++] = ar[i++];
        else
            tmp[k++] = ar[j++];
    }

    while (i < mid)
        tmp[k++] = ar[i++];
    while (j < len)
        tmp[k++] = ar[j++];

    std::copy(tmp, tmp+k, ar);
}


template<typename T>
void mergesort_int(T* ar, size_t len, T* tmp)
{
    if (len < 2)
        return;

    mergesort_int(ar, len/2, tmp);
    mergesort_int(ar+len/2, len-len/2, tmp+len/2);
    merge(ar, len/2, len, tmp);
}

template<typename T>
void mergesort(T* ar, size_t len)
{
    if (len<2)
        return;

    std::vector<T> tmp(len)
    mergesort_int(ar, len, &tmp[0]);
}

Using the same main() from the previous sample, the outcome is similar:

24 9 26 93 35 75 99 95 49 58 32 37 30 88 55 13 76 23 99 90 50 66 38 8 3 
3 8 9 13 23 24 26 30 32 35 37 38 49 50 55 58 66 75 76 88 90 93 95 99 99 

Remember, this now requires one single allocation N items (in our case we use a vector for the automatic memory management) which is then used throughout the algorithm. You will find this is considerably faster, particularly for large sets of data.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141