If you're after minimal changes to your approach, you could use the memmove()
function, which is potentially faster than your own manual version. You can't use memcpy()
as advised by one commenter as the areas of memory aren't permitted to overlap (the behaviour is undefined if they do).
There isn't a lot else you can do without changing the type of your storage or your algorithm. However, if you change to using a linked list then the operation becomes significantly more efficient, although you will be doing more memory allocation. If the allocation is really a problem (and unless you're on a limited embedded system it probably isn't) then pool allocators or similar approaches could help.
EDIT: Re-reading your question, I'm guessing that you're not actually using Heapsort, you just mean that your array was allocated on the heap (i.e. using malloc()
) and you're doing a simple insertion sort. In which case, the information below isn't much use to you directly, although you should be aware that insertion sort is quite inefficient compared to a bulk insert followed by a better sorting algorithm (e.g. Quicksort which you can implement using the standard library qsort()
function). If you only ever need the lowest (or highest) item instead of full sorted order then Heapsort is still useful reading.
If you're using a standard Heapsort then you shouldn't need this operation at all - items are appended at the end of the array, and then the "heapify" operation is used to swap them into the correct position in the heap. Each swap just requires a single temporary variable to swap two items - it doesn't require anything to be shuffled down as in your code snippet. It does require everything in the array to be the same size (either a fixed size in-place string or, more likely, a pointer), but your code already seems to assume that anyway (and using variable length strings in a standard char
array would be a pretty strange thing to be doing).
Note that strictly speaking Heapsort operates on a binary tree. Since you're dealing with an array I assume you're using the implementation where a contiguous array is used where the children of node at index n
are stored at indicies 2n
and 2n+1
respectively. If this isn't the case, or you're not using a Heapsort at all, you should explain in more detail what you're trying to do to get a more helpful answer.
EDIT: The following is in response to you updated code above.
The main reason you see a problem during deallocation is if you trampled some memory - in other words, you're copying something outside the size of the area you allocated. This is a really bad thing to do as you overwrite the values that the system is using to track your allocations and cause all sorts of problems which usually result in your program crashing.
You seem to have some slight confusion as to the nature of memory allocation and deallocation, first of all. You allocate an array of char*
, which on its own is fine. You then allocate arrays of char
for each string, which is also fine. However, you then just call free()
for the initial array - this isn't enough. There needs to be a call to free()
to match each call to malloc()
, so you need to free each string that you allocate and then free the initial array.
Secondly, you set sizeToMove
to a multiple of sizeof(MAX_STRING_SIZE)
, which almost certainly isn't what you want. This is the size of the variable used to store the MAX_STRING_SIZE
constant. Instead you want sizeof(char*)
. On some platforms these may be the same, in which case things will still work, but there's no guarantee of that. For example, I'd expect it to work on a 32-bit platform (where int
and char*
are the same size) but not on a 64-bit platform (where they're not).
Thirdly, you can't just assign a string constant (e.g. "hello world"
) to an allocated block - what you're doing here is replacing the pointer. You need to use something like strncpy()
or memcpy()
to copy the string into the allocated block. I suggest snprintf()
for convenience because strncpy()
has the problem that it doesn't guarantee to nul-terminate the result, but it's up to you.
Fourthly, you're still using memcpy()
and not memmove()
to shuffle items around.
Finally, I've just seen your comment that you have to use new
and delete
. There is no equivalent of realloc()
for these, but that's OK if everything is known in advance. It looks like what you're trying to do is something like this:
bool addItem(const char *item, char *list[], size_t listSize, size_t listMaxSize)
{
// Check if list is full.
if (listSize >= listMaxSize) {
return false;
}
// Insert item inside list.
for (unsigned int i = 0; i < listSize; ++i) {
if (strcmp(list[i], item) > 0) {
memmove(list + i + 1, list + i, sizeof(char*) * (listSize - i));
list[i] = item;
return true;
}
}
// Append item to list.
list[listSize] = item;
return true;
}
I haven't compiled and checked that, so watch out for off-by-one errors and the like, but hopefully you get the idea. This function should work whether you use malloc()
and free()
or new
and delete
, but it assumes that you've already copied the string item
into an allocated buffer that you will keep around, because of course it just stores a pointer.
Remember that of course you need to update listSize
yourself outside this function - this just inserts an item into the correct point in the array for you. If the function returns true
then increment your copy of listSize
by 1 - if it returns false
then you didn't allocate enough memory so your item wasn't added.
Also note that in C and C++, for the array list
the syntax &list[i]
and list + i
are totally equivalent - use the first one instead within the memmove()
call if you find it easier to understand.