You have a couple of problems. First, your look and if
conditions invoke Undefined Behavior by reading outside the bounds of v
in
for(j=n;j>=0;j--){
if(v[j]==v[j-1])
When j = n
you read 1-past the end of the array, and where j = 0
, you read 1-before the beginning of the array. A better approach would be:
i = n - 1; /* set i to last element */
while (i--) /* loop removing the duplicates */
/* if current equals next */
if (v[i] == v[i + 1]) {
...
Next, rather than using realloc
to trim block from the end, you should use memmove
to remove duplicates. (realloc
can do nothing for 1, 2, 2, 3, 4
) Instead simply backup over each element, checking for duplicates, and moving the end elements forward to overwrite the duplicate. Don't worry about realloc
until you have removed all duplicates -- then realloc
once, e.g.
i = n - 1; /* set i to last element */
while (i--) /* loop removing the duplicates */
/* if current equals next */
if (v[i] == v[i + 1]) { /* use memmove - blocks overlap */
memmove (&v[i], &v[i + 1], (n - i - 1) * sizeof *v);
n--; /* decrement count */
}
/* final reallocation */
tmp = realloc (v, n * sizeof *v); /* never realloc to original ptr */
if (tmp == NULL) { /* validate reallocation */
perror ("realloc-final");
return 1;
}
v = tmp; /* then assign new block to original pointer */
note: do not realloc
to the original pointer itself. Instead use a temporary pointer, and then validate that the realloc
succeeded before assigning the address for the new block of memory to the original pointer.
Putting together a short example that uses an initialized array of jumbled numbers rather than reading from a file (since none was provided), you could do something similar to:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* simple qsort compare - sort int ascending */
int compare (const void *a, const void *b)
{
return (*(int*)a > *(int*)b) - (*(int*)a < *(int*)b);
}
int main (void) {
int a[] = {5,1,5,5,2,8,3,2,6,9,4,8,1,4,3,6,9,2,7,7}, /* numbers */
n = sizeof a / sizeof *a, /* number of elements */
*v = NULL, /* pointer to allocate */
i;
void *tmp; /* temp pointer to realloc, never realloc w/original */
v = malloc (n * sizeof *v); /* don't cast return of malloc, always
* use 'n * sizeof dereferenced pointer'
*/
memcpy (v, a, n * sizeof *v); /* copy a to v */
qsort (v, n, sizeof *v, compare); /* sort v */
for (i = 0; i < n; i++){ /* output sorted v */
printf ("a[%2d]: %d\n", i, v[i]);
}
i = n - 1; /* set i to last element */
while (i--) /* loop removing the duplicates */
/* if current equals next */
if (v[i] == v[i + 1]) { /* use memmove - blocks overlap */
memmove (&v[i], &v[i + 1], (n - i - 1) * sizeof *v);
n--; /* decrement count */
}
/* final reallocation */
tmp = realloc (v, n * sizeof *v); /* never realloc to original ptr */
if (tmp == NULL) { /* validate reallocation */
perror ("realloc-final");
return 1;
}
v = tmp; /* then assign new block to original pointer */
printf ("Retirados:\n");
for (i = 0; i < n; i++){
printf ("a[%2d]: %d\n", i, v[i]);
}
free (v); /* don't forget to free memory you allocate */
#if defined (_WIN32) || defined (_WIN64)
getchar(); /* just use getchar() to hold terminal open on windows */
#endif
return 0;
}
(note: qsort
was used in place of your OrdenarQuicksort
since you did not provide the OrdenarQuicksort
code)
Example Use/Output
$ ./bin/array_rm_dups
a[ 0]: 1
a[ 1]: 1
a[ 2]: 2
a[ 3]: 2
a[ 4]: 2
a[ 5]: 3
a[ 6]: 3
a[ 7]: 4
a[ 8]: 4
a[ 9]: 5
a[10]: 5
a[11]: 5
a[12]: 6
a[13]: 6
a[14]: 7
a[15]: 7
a[16]: 8
a[17]: 8
a[18]: 9
a[19]: 9
Retirados:
a[ 0]: 1
a[ 1]: 2
a[ 2]: 3
a[ 3]: 4
a[ 4]: 5
a[ 5]: 6
a[ 6]: 7
a[ 7]: 8
a[ 8]: 9
Memory Use/Error Check
In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.
It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.
For Linux valgrind
is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.
$ valgrind ./bin/array_rm_dups
==26625== Memcheck, a memory error detector
==26625== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==26625== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==26625== Command: ./bin/array_rm_dups
==26625==
a[ 0]: 1
a[ 1]: 1
a[17]: 8
<snip>
a[18]: 9
a[19]: 9
Retirados:
a[ 0]: 1
a[ 1]: 2
a[ 2]: 3
a[ 3]: 4
a[ 4]: 5
a[ 5]: 6
a[ 6]: 7
a[ 7]: 8
a[ 8]: 9
==26625==
==26625== HEAP SUMMARY:
==26625== in use at exit: 0 bytes in 0 blocks
==26625== total heap usage: 2 allocs, 2 frees, 116 bytes allocated
==26625==
==26625== All heap blocks were freed -- no leaks are possible
==26625==
==26625== For counts of detected and suppressed errors, rerun with: -v
==26625== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Always confirm that you have freed all memory you have allocated and that there are no memory errors.
Validate All File Opens & Reads
Reading from a file, you must validate the fopen
and you must validate each fscanf
. There is no reason to include the '\n'
in your format string, the "%d"
(and in fact all numeric) format specifiers consume leading whitespace (which includes ' '
, '\t'
and '\n'
).
When using malloc
or realloc
, do not cast the result -- there is no reason to. Both return void*
which can be assigned to any pointer type without a cast, see: Do I cast the result of malloc?
Putting those pieces together, your open and read could be done something like:
f = fopen ("InteirosComRepetidos.txt","r");
if (f == NULL) { /* validate file open for reading */
perror ("fopen-InteirosComRepetidos.txt");
return 1; /* if it isn't, exit */
}
v = malloc (sizeof *v); /* don't cast return of malloc, and if you always
* use 'sizeof dereferenced pointer', times some
* amount, you will always get your allocations
* correct */
while (fscanf (f, "%d", &v[n]) == 1) { /* loop on validated scanf return */
tmp = realloc (v, (n+1) * sizeof *v); /* never realloc ptr iself */
if (tmp == NULL) { /* validate realloc succeeded/handle failure */
perror ("realloc-v");
return 1;
}
v = tmp; /* assign reallocated pointer to v */
n++; /* advance counter */
}
note: you never realloc
and assign the result to the original pointer (e.g. v = realloc (v, ...)
). realloc
can fail and when it does, it returns NULL
. In that case, you have just lost the reference to your original block of memory -- resulting in not being able to use or free
that memory (you have created a memory leak). Instead, always realloc
with a temporary pointer (e.g. void *tmp = realloc (v, ...)
) and validate that the reallocation succeeds before assigning v = tmp;
. That way, in the event realloc
fails, you still have your reference to the original block of memory in v
(which still holds (or points to) all your original values before your attempted realloc
)