At main, if you add a printf("*** n=%d\n", n)
before each call to print
, you will notice that, before the second call, the output is n=61
. That is, you are sorting the array well, but by the time you print it a second time, you print 61 numbers.
You can notice as well that 61 is the biggest number in the array, and that n
is defined just after arr
, so n
and arr
will be in consecutive memory addresses in the stack. I think n
is being overwritten during the mergesoft
function call.
Effectively, the overwritting happens when you call mergesoft
with &arr[n]
. The last element of the array has the index n-1. The nth index actually corresponds to the n
variable. Change the call to: mergesort(arr,&arr[0],&arr[n-1]);
EDIT
Your edits made my original answer (below) nonsensical to other readers :) Oh, well...
First thing is to decide if Your begin
and end
pointers define a range [begin, end) or [begin, end]. I suggest the first choice because it is used in C++ library. If You agree with that, the call
mergesort(arr,&arr[0],&arr[n]);
is correct. You have to change &arr[n]
to &arr[n-1]
only if You decide, You want the pointers begin
and end
to define the range [begin, end] which I suggest not to do.
In fact the pointers begin
and end
are enough to sort the range and Your mergesort
function doesn't need the first parameter.
The calculation of med
is incorrect
med=arr[arr_end-arr_begin]/2+arr_begin;
It was correct in the previous version (the variable was named q)
The calls below are also incorrect
mergesort(arr,arr_begin,med);
mergesort(arr,med+1,arr_end);
First call is supposed to sort the range [arr_begin, med) , not [arr_begin, med] (*med
doesn't belong to that range), so the second call should sort the range starting at med
.
The allocation of arr1 is correct, thanks to the fact, that the difference end - begin
is equal to the number of elements. That's the advantage of picking the range [begin, end) instead of [begin, end]. It would be nice however, if You freed the allocated memory after use.
The call to merge is incorrect, like the calls to mergesort
above. The second range sholuld start at med
because med
points past the first range, not at it's last element.
The implementation of merge
is overcomplicated. You don't have to swap anything. You just take elements from both ranges and copy them to the destination. Those three while loops that started my original post (below) are enough, but pay attention to the conditions.
I repeat once again med
points past the first range and arr_end
points past the second range. Taking it into consideration, should You use <=
or <
operators?
I don't like the inconsistency in the conditions i<=q
, j<=u
, i<q
, j<u
in the following code:
while(i<=q && j<=u){
if(*i<*j){
*k=*i;
i=i+1;
}
else{
*k=*j;
j=j+1;
}
k=k+1;
}
while(i<q){*k=*i;i++;k++;}
while(j<u){*k=*j;j++;k++;}
You call Your mergesort like this:
mergesort(arr,&arr[0],&arr[n]);
which means, that u
is a pointer, that points to the spot after the last element of Your array. In other words, You seem to think of Your pointers, like of iterators begin
and end
which define the range [begin, end) - *begin
belongs to the range, but *end
not,
In the definition of mergesort
You write
int *q=((u-p)/2)+p;
mergesort(arr,p,q);
mergesort(arr,q+1,u);
which is inconsistent with the assumptions above. q
may be equal p
if u == p+1
.
mergesort(arr,p,q);
means sort the range [p, q) (q
is past the range)
mergesort(arr,q+1,u);
means sort the range [q+1, u) (u
is past the range)
If You were consistent in Your representation of a range with pointers, You would never touch the element *q
this way.
Thinking of the range as [begin, end) instead of [begin, end] (in the second case *end
is part of the range) is consistent with the way iterators are used in C++, but it's not obligatory. You can use pointers to define the range also the second way, but in that case, You have to change the call mergesort(arr,&arr[0],&arr[n]);
to mergesort(arr,&arr[0],&arr[n-1]);
. In both cases You have to rethink conditions in the code cited at the beginning.
This is a homework, so I won't solve it for You, but here is a little hint, that might help thinking about it. Redefine Your merge
so it takes 2 ranges, to merge, explicitly:
void merge(int * destination, int * r1_begin, int * r1_end, int * r2_begin, int * r2_end);
and think how to use it. Later You can simplify things. You don't really need destination parameter, and You don't have to copy all merged elements. You can copy only one range first and then, merge directly into destination, taking elements from the copy of the first range and from the second range.