Invalid assignment on dynamic array C

Question

Hello i am making a program in C which stores integers on a dynamic array which uses realloc every time it has to add a new element, i declare the array on the main:

int *abundants;

int count = abundant_numbers(&abundants);

once finished, i want to pass the modified array to another function to make other calculations

int abundant_numbers(int *abundants[]){
    if (!(*abundants = (int*) malloc(sizeof(int)))){
        perror("malloc error!\n");
        exit(EXIT_FAILURE);
    } 

    *abundants[0] = 12;     //we know the very first abundant number
    int count = 1, n = 14;  

    while (n < MAX_NUM){
        if (is_abundant(n)) {
            if (!(*abundants = (int*) realloc(*abundants,(count+1) * sizeof(int)))){
                perror("Error in realloc\n");
                exit(EXIT_FAILURE);
            }

            *abundants[count] = n;
            count++;
        }
        n += 2;     //no odd abundant numbers
    }

    return count;
}

the first time it enters on the if statement gives no problems, but the second time on the assignment i get a Segmentation Fault: 11, when accesing abundants[2], i dont understand why its not a valid position if it worked fine for abundants[1]

Thanks.

Could you fix the formatting while you make a minimal reproducible example, please? — Yunnosch
– Yunnosch, Commented Jul 12, 2017 at 6:31
"a dynamic array which uses realloc every time it has to add a new element" -- not your question, but this is a bad allocation strategy with unnecessary overhead. Always realloc in chunks, an often used strategy is to double the capacity once you run out (whether this is the best depends on your usecase). — user2371524
– user2371524, Commented Jul 12, 2017 at 7:06
@SGeorgiades, is_abundant will either return 0 or 1 always, problem lies on the assigment — Dan14
– Dan14, Commented Jul 12, 2017 at 8:01

score 1 · Accepted Answer · 2017-07-12 07:55:55Z

Your problem is a simple one in these lines:

*abundants[0] = 12;

*abundants[count] = n;

The indexing operator [] has higher precedence than the dereference operator *. So here you're treating your abundants as an array pointer directly and try to dereference the element. What you want instead is

(*abundants)[0] = 12;

(*abundants)[count] = n;

This should solve your problem, the remaining code will work correctly.

That being said, I would strongly suggest to use some data structure like this:

struct dynarr
{
    size_t count;
    size_t capacity;
    int entries[];
}

and realloc() in larger chunks, always when your count reaches your capacity. realloc() is costly and you risk fragmenting your heap space in a typical heap-based implementation. Your code could look for example like this:

#include <stdio.h>
#include <stdlib.h>

#define MAX_NUM 1024

int is_abundant(int x) { return x; } // simple fake to make it compile, replace

struct dynarr
{
    size_t count;
    size_t capacity;
    int entries[];
};

struct dynarr *createarr(size_t capacity)
{
    struct dynarr *arr = malloc(sizeof(*arr) + capacity * sizeof(int));
    if (!arr)
    {
        perror("malloc error!\n");
        exit(EXIT_FAILURE);
    }
    arr->count = 0;
    arr->capacity = capacity;
    return arr;
}

struct dynarr *expandarr(struct dynarr *arr)
{
    size_t capacity = arr->capacity * 2;
    struct dynarr *newarr = realloc(arr,
            sizeof(*newarr) + capacity * sizeof(int));
    if (!newarr)
    {
        perror("malloc error!\n");
        free(arr);
        exit(EXIT_FAILURE);
    }
    newarr->capacity = capacity;
    return newarr;
}

struct dynarr *abundant_numbers(void){
    struct dynarr *abundants = createarr(32);
    abundants->entries[abundants->count++] = 12; //we know the very first abundant number

    int n = 14;

    while (n < MAX_NUM){
        if (is_abundant(n)) {
            if (abundants->count == abundants->capacity)
            {
                abundants = expandarr(abundants);
            }
            abundants->entries[abundants->count++] = n;
        }
        n += 2;     //no odd abundant numbers
    }

    return abundants;
}

int main(void)
{
    struct dynarr *abundants = abundant_numbers();

    for (size_t i = 0; i < abundants->count; ++i)
    {
        printf("%d ", abundants->entries[i]);
    }
    free(abundants);
    putchar('\n');
}

Thanks for the suggestions, i will follow your recommendation and change my whole implementation

user3629249 · Accepted Answer · 2017-07-12 20:07:12Z

0

the biggest problem is that the code is expecting an array of pointers to int.

But the code is only producing an array of `int`s

And the code contains several 'magic' numbers (2, 12, 14)


int *abundants = NULL;

int count = abundant_numbers(&abundants);



int abundant_numbers(int *abundants[])
{

     if (!( abundants = malloc(sizeof(int))))
     {
         perror("malloc error!\n");
         exit(EXIT_FAILURE);
     } 

     abundants[0] = 12;     //we know the very first abundant number
     int count = 1;
     int n = 14;

     while (n < MAX_NUM)
     {
        if (is_abundant(n))
        {
            void *temp;
            if (!( temp = realloc(abundants,(count+1) * sizeof(int))))
            {
                perror("Error in realloc\n");
                free( abundants );
                exit(EXIT_FAILURE);
            }

            // implied else, realloc successful

            abundants = temp;
            abundants[count] = n;
            count++;
        }
        n += 2;     //no odd abundant numbers
    }

    return count;
}

However, since MAX_NUM is a known value, it would be better to just allocate that much memory in the beginning.

And strongly suggest to NOT have 'special' code for special cases of the value of 'n'.

And give 'magic' numbers meaningful names, suggest via #define statements.

sample code follows:

#include <stdlib.h>   // malloc(), free()

// use whatever value your program needs in the following statement.
#define MAX_NUM 1024
#define FIRST_ABUNDANT 12
#define STEP_AMOUNT 2

// prototypes
int abundant_numbers( int * );


int main( void )
{
     int *abundants = NULL;

     if (!( abundants = malloc(sizeof(int) * MAX_NUM)))
     {
         perror("malloc error!\n");
         exit(EXIT_FAILURE);
     } 

     // implied else, malloc successful

    int count = abundant_numbers( abundants  );
} // end function: main


int abundant_numbers( int *abundants )
{
     int count = 0;

     for( int n=FIRST_ABUNDANT; n < MAX_NUM; n+=STEP_AMOUNT )
     {
        if (is_abundant(n))
        {
            abundants[count] = n;
            count++;
        }
    }

    return count;
}  // end function: abundant_numbers

answered Jul 12, 2017 at 20:07

user3629249

16.6k1 gold badge18 silver badges17 bronze badges

2 Comments

Dan14 Over a year ago

i agree you with the #define, i use it a lot, but not in this case, this is just a small program so i forgot to use, it was my bad not to put my whole code to avoid missunderstandings, also, there are odd abundant numbers!, my error again! Anyways i finished the problem successfully. Thanks for your time

user3629249 Over a year ago

@Dan14, big program, small program, makes no difference. bad programming habits should never be encouraged.

Collectives™ on Stack Overflow

Invalid assignment on dynamic array C

2 Answers 2

1 Comment

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related