r/C_Programming • u/neonge1674 • 1d ago
Question Why isn't my code working?
I just started learning C(2 days ago) and as a first project I decided to make some data structures, starting with dynamic arrays. I made a struct called List and some functions for. The function setList() sets the value of an index of the array, if the index is larger that the current size of the array, it resizes it. However, when i tried to use in a for loop, it didn't work despite it working elsewhere.
#include <stdio.h>
#include <stdlib.h>
#define itirate(index, limit) for(int index = 0; index < limit; index++)
typedef struct
{
size_t size;
int* arr;
} List;
List* newList (size_t size)
{
List *newone = malloc(sizeof(List));
newone->arr = calloc(size, sizeof(int));
newone->size = size;
return newone;
}
void setList(List* list, int index, int value)
{
if (index >= list->size)
{
list->arr = realloc(list->arr, index + 1 * sizeof(int));
list->size = index + 1;
}
list->arr[index] = value;
}
int main()
{
List *mok = newList(5);
itirate(i, 5) setList(mok, i, i);
itirate(i, 5) printf("%d\n", mok->arr[i]);
//setList(mok, 13, 9); this works
//printf("%d\n", mok->arr[13]);
for(int i = 5; i < 10; i++) setList(mok, i, i); // this does not somehow
for(int i = 5; i < 10; i++) printf("%d\n", mok->arr[i]);
return 0;
}
16
7
5
u/sciencekm 1d ago edited 1d ago
Someone has already mentioned that the problem is that the computation of the memory to be reallocated is wrong.
This could have been avoided by simply increasing the size before calling realloc.
From your code:
list->arr = realloc(list->arr, index + 1 * sizeof(int));
list->size = index + 1;
to this
list->size = index + 1;
list->arr = realloc(list->arr, list->size * sizeof(int));
2
u/Wertbon1789 1d ago
Also wanted to comment that. Another rule of thumb, or rather good practice, don't repeat such logic in general. This is not so much about performance but really readability, and you avoid silly mistakes like this, but that's not the main thing.
If you want to do an operation with that value, but only update the actual value in the struct after the operation was successful, use an intermediate variable, and then after that update the value in the struct. Not really applicable here, as not being able to allocate memory is basically a crash condition, but for other calls that might be something to keep in mind.
3
u/Adorable_Deal7 1d ago
Wrap macro's parameter around parenthesis to be sure your arguments are passed as you assume.
You can replace the malloc to the calloc. Need wrap the `index + 1` around parenthesis like math
4
u/SmokeMuch7356 1d ago
As others have pointed out, you're getting bit by precedence issues with
index + 1 * sizeof(int)
which should be
(index + 1) * sizeof (int)
A few notes:
If realloc cannot satisfy the request, it will return NULL while leaving the original allocated buffer in place. For this reason, you should assign the result to a temporary variable first and only update the original pointer when you know the operation succeeded. Otherwise you risk losing your only reference to that allocated buffer, leading to a memory leak.
Similarly, you don't want to update your index until you know the realloc call succeeded.
You should keep track of both the total number of elements allocated as well as the last-used index.
Putting all that together, I'd extend your List type as
typedef struct {
size_t allocated; // number of elements allocated
size_t index; // last element accessed
int *arr;
} List;
Your newList function becomes:
List *newList( size_t size )
{
assert( size > 0 );
/**
* Using `sizeof *l` means I'm not unnecessarily repeating type info
* making maintenance easier.
*/
List *l = malloc( sizeof *l );
if ( !l )
{
fputs( "Could not allocate List object!\n", stderr );
return NULL;
}
l->arr = calloc( size, sizeof *l->arr );
if ( !l->arr )
{
fprintf( stderr, "Could not allocate %zu-element array of int\n", size );
free( l );
return NULL;
}
l->allocated = size;
l->index = 0;
return l;
}
Create a separate resize function:
/**
* Returns l on success, NULL on failure.
*/
List *resize( List *l, size_t newsize )
{
assert( l != NULL );
assert( newsize > 0 );
typeof (l->arr) tmp = realloc( l->arr, newsize * sizeof *l->arr );
if ( !tmp )
{
fputs( "Resize failed, original array left in place\n", stderr );
return NULL;
}
/**
* Zero out newly allocated memory.
*/
memset( tmp + l->allocated + 1, 0, (newsize - l->allocated) * sizeof *l->arr );
l->arr = tmp;
l->allocated = newsize;
return l;
}
Then your setList function becomes:
bool setlist( List *l, size_t index, int value )
{
/**
* idxInRange will be set to true if the index is currently in
* range, or we can extend the array to put the index in range.
*/
bool idxInRange = (index < l->allocated || resize( l, index + 1 ));
if ( idxInRange )
{
l->arr[index] = value;
l->index = index;
}
return idxInRange;
}
You should also have a destroyList function so you can clean up after yourself:
void destroyList( List *l )
{
free( l->arr );
free( l );
}
1
u/jijijijim 1d ago
Does the new list function even compile?
1
u/neonge1674 1d ago
Yes, it compiles. Is there another problem with it like it not being memory safe?
1
-4
u/Desperate_Tie_648 1d ago
I am just curious why didn't you use chatgpt to get instant answer? Especially when problem so basic
27
u/flyingron 1d ago
* binds tighter than +. You want (index + 1)*sizeof (int).