0

I have this C function:

fill_array(&data, &size);

void fill_array(int **data, int *size){
   printf("Size is:");
   scanf("%d", size);
   *data = malloc(*size * sizeof(int *));
   int i = 0;
   for (i = 0; i < size; i++){
      (*data)[i] = rand() % 11;
   }
}

I want to assign data[i] for example, to random number. How to do such a thing? I have tried many variations, but all of the time my program crashes. Thanks.

  • See [operator precedence](http://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence) – Shahbaz Apr 16 '13 at 13:58
  • why are you passing the size when you are asking it from user ? – Amogh Talpallikar Apr 16 '13 at 13:58
  • Side note: Write `type *var = malloc(size * sizeof(*var));` instead of `type *var = (type *)malloc(size * sizeof(type));`. The first one is much cleaner, and much more maintainable. If you change the type of `var`, you don't run the risk of allocating wrong size of memory because you forgot to change `type` everywhere. In short, don't repeat yourself. – Shahbaz Apr 16 '13 at 14:00
  • @marius, right now, you have `sizeof(int *)` in your `malloc` which should have been `sizeof(int)`. If you had listened to my previous comment, this error wouldn't have happened. – Shahbaz Apr 17 '13 at 07:54

4 Answers4

3
*data = malloc(*size * sizeof(**data));
(*data)[5] = 15;

Refer to cdecl web site.

Do not cast malloc

Edit according to the question edit

the for loop contains typo

for (i = 0; i < size; i++)

it should be

for (i = 0; i < *size; i++)
Community
  • 1
  • 1
MOHAMED
  • 41,599
  • 58
  • 163
  • 268
  • 1
    It should be `*size`, not `size`, as `size` is `int*`, not `int` in the function. Also: don't cast the result of `malloc`, as it should be `void*` anyway. – Dirk Apr 16 '13 at 13:59
  • Ok, so: void fill_array(int **data, int *size){ printf("Size is:"); scanf("%d", size); *data = malloc(*size * sizeof(int*)); int i = 0; for (i = 0; i < size; i++){ (*data)[i] = rand() % 11; } } Program still crashes –  Apr 16 '13 at 14:32
  • @Marius *data = malloc(*size * sizeof(int *)); – MOHAMED Apr 16 '13 at 14:34
  • @Marius update your question with the new code instead of writing it in the comment – MOHAMED Apr 16 '13 at 14:35
  • @Marius the function definition in your new code is wrong you have changed it to `void fill_array(int *data, int *size)` it should be `void fill_array(int **data, int *size)` – MOHAMED Apr 16 '13 at 14:40
  • Found the issue. Had to use *size when declaring for cycle. Thanks. –  Apr 16 '13 at 14:43
  • @Marius the use of `size` in your `for` loop is wrong it should be `for (i = 0; i < *size; i++)` – MOHAMED Apr 16 '13 at 14:44
  • @unkulunkulu - I consider it a little bit rude to edit an answer "while it is still warm". And Mohamed did a good job of fixing it himself. – Dirk Apr 16 '13 at 21:10
1

you can use (*data)[5] = 15; instead of this *data[5] = 15; Because precedence of [] greater than precedence of *..

Mani
  • 17,549
  • 13
  • 79
  • 100
1

As others said, you need to put parentheses to get the operator precedence right. If you want to use the "array" a lot, it might make sense to create a temporary variable that is easy to use:

int *p;
...
*data = malloc(*size * sizeof **data);
p = *data;

And then you could use p[5] etc.

Alok Singhal
  • 93,253
  • 21
  • 125
  • 158
0

Good program design dictates that we should keep memory allocation and the actual algorithm separated. To have a function that takes user input and allocates memory and performs some algorithm is probably not the optimal program design.

So the proper solution is not to patch that function to make it work, but instead to make some new ones:

int get_size_from_user (void)
{
  int size;
  printf("Size is:");
  scanf("%d", &size);
  return size;
}

bool alloc_array (int** array, int size)
{
  *array = malloc(size * sizeof(int));
  return *array != NULL;
}

void fill_array (int* array, int size)
{
  // ...whatever you want to do here
  data[5] = 15;
}

And look at that, the need for obscure syntax disappeared as soon as we improved the program design! Coincidence?

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • @Shahbaz Whether to use `sizeof(int)` or `sizeof(**array)` is _coding style_ and thus subjective. There is no consensus over which form that is the better. No matter, with your reputation you should know better than to edit posts to change the content of source code. – Lundin Apr 17 '13 at 08:53
  • on the contrary, there _is_ a consensus over which form is better. `x = malloc(sizeof *x)` is always correct. `c = malloc(sizeof(some_type))` can go wrong. Since `malloc` doesn't care for the type, it can't diagnose problems if the type mismatches what you expect. Compilers don't help you either. `sizeof(type)` doesn't make the code any clearer either. There is no tradeoff here. Not repeating the type is the clear better choice. – Shahbaz Apr 17 '13 at 09:17
  • @Shahbaz That's nonsense. If I write `sizeof(wrong_type)` or `sizeof(*array)` in the above example, the program will crash equally bad. Why would it be easier to write the wrong type, instead of writing the wrong number of indirections (**)? This is subjective coding style, end of story. – Lundin Apr 17 '13 at 11:17
  • @Shahbaz And it really doesn't matter, because you **do not silently edit the content of other people's code**. If you find something wrong about it, you write a comment and let the poster fix it. I shouldn't have to explain this to a 15k rep user. But in case you have never done edit reviews before, [read this](http://meta.stackexchange.com/questions/155538/review-guidelines) and [this](http://meta.stackexchange.com/questions/155538/review-guidelines/155539#155539). – Lundin Apr 17 '13 at 11:25
  • Didn't SO notify you about the edit? It's not like I silently edited the content. I didn't change the semantics of your answer, and my edit didn't make it wrong or harm the answer in any way. And that is knowing that SO would notify you about it anyway. Back on topic, if you write `sizeof(wrong_type)` or `sizeof(*array)` you would have to debug and fix it either way, but later if you decided to change the type of `array` for whatever reason, you don't need to go about changing that type in many places; only changing the definition would suffice. In your case, you are prone to another crash. – Shahbaz Apr 17 '13 at 12:34
  • In short, in a function like `malloc` where there is no way for the compiler to do your type checking, why not let it automatically calculate the right thing based on whatever the type the array has, rather than manually write the type yourself? Neither has any run-time cost and the compilation cost is extremely minor. – Shahbaz Apr 17 '13 at 12:36
  • @Shahbaz C has a feature called `typedef`. One might as well argue and say that a typedef is much safer, since you only need to change one place in the code. While the indirection is dangerous because you can get the levels of indirection wrong. In a function like the above, it will be `sizeof(**array)`, outside the function it will be `sizeof(*array)`. In the case of an array of pointers, it will be `sizeof(***array)`. The potential for bugs is obvious. As you can tell solely from the amount of arguments for either style, this is *subjective coding style*. Neither style is obviously better. – Lundin Apr 17 '13 at 12:54