current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

This might be a basic question for C. I'm making a struct called check_t and I want to make an array whose elements are of type check_t. This array is called enqueued. The struct check_t contains an integer x and a lock (for multi-threading).

typedef struct _check_t{
  int x;
  pthread_mutex_t lock;
} check_t;

void *check_init(check_t *c)
  c->x = 0;
  pthread_mutex_init(&c->lock,NULL);
}

check_t **enqueued;
for(i=0;i<w*h;i++){
  enqueued[i] = (check_t *)malloc(sizeof(check_t));
  check_init(&enqueued[i]);
}

Mostly, I'm wondering if this is the correct way to make/allocate space for an array of check_ts.

share|improve this question

closed as off-topic by Michael Urman, Lyle's Mug, William Morris, svick, Jeff Vanzella Dec 9 '13 at 17:09

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – Michael Urman, Lyle's Mug, William Morris, svick, Jeff Vanzella
If this question can be reworded to fit the rules in the help center, please edit the question.
    
check_t **enqueued; is how you'd declare a pointer to a 2D array of check_t. Is that what you want? –  Fiddling Bits Dec 9 '13 at 0:46
    
enqueued is supposed to be a 1D array. However, I know that I need to stars; one to make it an array, and the other to indicate that I want pointers to each check_t. I can't do enqueued[i] if I say check_t *enqueued. –  user33187 Dec 9 '13 at 1:05
    
*enqueued and enqueued[0] mean the same thing. –  Fiddling Bits Dec 9 '13 at 1:11
1  
You probably want the return type of check_init to be defined as void and not void* as well, since you're not returning anything from that method. –  Matt Dec 9 '13 at 2:28

2 Answers 2

up vote 5 down vote

1D array:

check_t *enqueued = malloc(N * sizeof(*enqueued));
enqueued[0].x = 5;
enqueued[N - 1].x = 2;

2D array:

check_t **enqueued = malloc(N * sizeof(*enqueued));
for(int i = 0; i < N; i++)
    enqueued[i] = malloc(M * sizeof(**enqueued));
enqueued[0][0].x = 5;
enqueued[N - 1][M - 1].x = 2;
share|improve this answer
    
I'm much more familiar with spelling the sizeof parts as sizeof(struct check_t) than sizeof(*[*]enqueued), but I've used similar approaches for countof style macros. Interesting. –  Michael Urman Dec 9 '13 at 1:10
    
It's safer in case enqueued changes types. From check_t to some other struct, for example. –  Fiddling Bits Dec 9 '13 at 1:14
    
@Bit Fiddling Code Monkey I concur about the A = malloc(N *sizeof *A) idiom. But then why not enqueued[i] = malloc(M * sizeof *(enqueued[i]))? –  chux Dec 9 '13 at 2:51
up vote 2 down vote

Firstly, decide whether your program actually needs to be C and not C++. It's much safer and generally easier to use a std::vector<check_t> than a manually allocated region, as std::vector does automatic memory management for you (it makes it hard to memory leak) and it makes it hard to index the vector outside of bounds, which would cause a potentially exploitable memory corruption vulnerability in your program.

If for some reason you must do this without the standard template, next decide whether you can't use new[] in C++. This will simplify your code and be more clear what you're doing:

void main()
{
   check_t* myArray = new check_t[count];
}

Finally, if you're absolutely positively 100% sure you can't use std::vector or new[], ask yourself if you can do it via calloc:

void main()
{
   check_t* myArray = calloc(nElements, sizeof(check_t));
}

But if, heaven forbid, you're doing some assignment that explicitly states that you must use malloc on pain of death that you ever use any of the multiple better alternatives that have so kindly been made available to you, this is how you do it:

#include <a_safe_int_library.h>
check_t* CreateAndInitializeCheckTArray
(
   /* IN */ size_t nElements
)
{
   size_t cbSize;
   check_t* array = NULL;
   size_t i;

   // avoid an exploitable integer/heap overflow first:
   if(SafeMultiply(&cbSize, sizeof(check_t), nElements) == OVERFLOW) 
       return NULL;

   array = (check_t*)malloc(cbSize);
   if(array == NULL)
      return NULL;

   // clear all of the elements:
   memset(array, 0, cbSize);

   // initialize the array:
   for(i = 0; i < nElements; i++)
   {
     check_init(&array[i]);
   }

   return array;
}
share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.