Dynamic Allocation and Reallocation of Memory

Question

This is a code snippet of a larger program. I tried to simplify it as much as possible.

This code identifies certain particles and stores them in a list which corresponds to a certain processor.
Besides that I count the number of particles per processor that are already in the list.
After the work is done I want to use the message-passing system MPI to send the particles in the list to other processors (not shown here).

I would like to know:

• Is the process of allocation and deallocation of memory correct?
• Is the code robust?
• Can this be simplified?

Code:

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

#define DIM 2

typedef struct{
    double m;   
    double x[DIM];
    double v[DIM];
    int id;
} Particle;

typedef struct{
    double **list;
    int *counter;
} ParticleList;

void generateParticles(Particle *p, int N);
void buildList(Particle *p, ParticleList *plist, int numprocs, int N);

int main() {
    time_t t;
    srand((unsigned)time(&t));

    // Generate data, print data
    int N = 50;
    Particle *p = (Particle*)malloc(N*sizeof(*p));
    generateParticles(p, N);
    for(int i=0; i<N; i++){
        printf("id: %d m: %lf x: %lf %lf v: %lf %lf\n", p[i].id, p[i].m, p[i].x[0], p[i].x[1], p[i].v[0], p[i].v[1]);
    }

    // Fill lists of every processor
    int numprocs = 4;
    ParticleList plist;
    plist.list = malloc(sizeof(double*)*numprocs);
    for(int k=0; k<numprocs; k++)
        plist.list[k] = malloc(sizeof(double)*1);
    plist.counter = calloc(numprocs, sizeof(int));
    buildList(p, &plist, numprocs, N);

    for(int k=0; k<numprocs; k++){
        printf("Processor %d Particles in list %d\n", k, plist.counter[k]);
        for(int c=0; c<(DIM*plist.counter[k]); c++){
            printf("%lf ", plist.list[k][c]);
        }
        printf("\n");
    }

    // deallocate memory
    for(int k=0; k<numprocs; k++){
        free(plist.list[k]);
    }
    free(plist.list);
    free(plist.counter);
    free(p);
    return 0;
}

void buildList(Particle *p, ParticleList *plist, int numprocs, int N){
    for(int k=0; k<numprocs; k++){
        for(int i=0; i<N; i++){
            printf("k %d, i %d\n", k, i);
            if(rand()%10 < 5){
                plist->counter[k] = plist->counter[k] + 1;
                plist->list[k] = realloc(plist->list[k], (DIM*plist->counter[k])*sizeof(plist->list[k][0]));
                int d=0;
                for(int j=(plist->counter[k] - 1)*DIM; j < (plist->counter[k] * DIM); j++){
                    plist->list[k][j] = p[i].x[d];
                    d++;
                }
            }
        }
    }
}

double frand(){
    return (double)rand() / (double)RAND_MAX; 
}

void generateParticles(Particle *p, int N){
    for(int i=0; i<N; i++){
        for(int d=0; d<DIM; d++){
            p[i].x[d] = frand();
            p[i].v[d] = frand();
        }
        p[i].m = frand();
        p[i].id = i;
    }
}

Answer 1

Code would have benefited with allocateList(), allocateParticles(), freeList(), freeParticles() functions, rather than trying coding it all up in main().

allocation and deallocation of memory correct?

Allocations lack error checking. This is the big issue as it also impacts proper de-allocation code.

plist->list[k] = realloc(plist->list[k], .... leak memory should the allocation fail.

DIM*plist->counter[k])*sizeof(plist->list[k][0]) could overflow int math, yet work well with size_t math. See re-ordering below.

Robust-ness

Use size_t instead of int for indexing. As "code snippet of a larger program", the array sizes may be very large. The best type for array indexing is size_t, an unsigned type.

// for(int d=0; d<DIM; d++){
for(size_t d=0; d<DIM; d++){
        p[i].x[d] = frand();
        p[i].v[d] = frand();
    }

Simplification

C does not require the cast nor () with sizeof object

// Particle *p = (Particle*)malloc(N*sizeof(*p));
Particle *p = malloc(N * sizeof *p);

// I prefer `sizeof` first, especially  for math that may benefit with `size_t` math.
//                   v---------------v all size_t math  
Particle *p = malloc(sizeof *p * N * N);
// vs                v---v may overflow `int` math
Particle *p = malloc(N * N * sizeof *p);

Simplify coding, review and maintenance. Is plist.list a double *? Why bother attempting to co-coordinate the type of member list with the malloc() call which may be in another file?

// plist.list = malloc(sizeof(double*)*numprocs);
plist.list = malloc(sizeof *(plist.list) * numprocs); 

// plist.counter = calloc(numprocs, sizeof(int));
plist.counter = calloc(numprocs, sizeof *(plist.counter));
// or 
plist.counter = calloc(numprocs, sizeof plist.counter[0]);

The *1 is curious and lacks benefit

// plist.list[k] = malloc(sizeof(double)*1);
plist.list[k] = malloc(sizeof *(plist.list[k]));

---

buildList() would benefit with re-write - IMO, is is not simple to follow. As code lacks guiding comments, I am left to guess how to simplify it without losing meaning. Sample idea with some style improvements.

// Style issue: Avoid wrapping presentation code and horizontal scroll bars
void buildList(Particle *p, size_t n, ParticleList *plist, size_t numprocs) {
  for (size_t np=0; np<numprocs; np++) {
    for (size_t i=0; i<n; i++){
      printf("np %zu, i %zu\n", np, i);
      if (rand()%10 < 5) {
        size_t new_count = plist->counter[k] + 1;
        void *new_ptr = realloc(plist->list[np],sizeof plist->list[np][0] * DIM*new_count);
        if (new_ptr == NULL) {
          TBD_ErrorHandler();
          // ...
        }
        plist->list[k] = new_ptr;
        plist->counter[k] = new_count;
        // I think the following is the same - I had trouble following original code.
        for(size_t dm = 0; dm < DIM; dm++) {
          plist->list[pn][new_count + j] = p[i].x[dm];
        }
      } // endif
    }
  }
}

"l" in "%lf" not required - more of a style issue

// printf("id: %d m: %lf x: %lf %lf v: %lf %lf\n", 
printf("id: %d m: %f x: %f %f v: %f %f\n", 
  p[i].id, p[i].m, p[i].x[0], p[i].x[1], p[i].v[0], p[i].v[1]);

---

Sample ParticleList_free()

void ParticleList_free(ParticleList *pl) {
  if (pl) {
    if (pl->list) {
       for(int k=0; k<numprocs; k++) {
         free(pl->list[k]);
         pl->list[k] = NULL; // ***
       }
       free(pl->list);
       pl->list = NULL; // ***
    }
    free(pl>counter);
    pl>counter = NULL; // ***
    free(pl);
  }

  // Sample usage
  ParticleList *pl = ParticleList_alloc();
  buildList(p, pl, numprocs, N);
  ....
  ParticleList_free(pl);
  pl = NULL; // ***

*** Useful for debugging, but not needed for production code. By setting to NULL, errant uses of pointers are typically more likely to be found. YMMV.