0

I'm a novice in C. I have a function with such a structure (this code is not reproducible, this is "pseudo-code", but I hope it's enough to explain my problem):

unsigned** myfunction(double*** A, unsigned nx, unsigned ny, unsigned nz){
    unsigned* cells[nz-1];
    unsigned totallength = 0;
    unsigned lengths[nz-1];
    unsigned** bottomTypes = function1(A, nx, ny, 0);
    for(unsigned k=0; k<nz-1; k++){
        unsigned** topTypes = function1(A, nx, ny, k+1);
        unsigned** cellTypes = function2(bottomTypes, topTypes);
        unsigned length
        unsigned** goodcells = function3(cellTypes, &length);
        cells[k] = malloc(length * sizeof(unsigned));
        for(unsigned l=0; l<length; l++){
            cells[k][l] = goodcells[0][l] + k;
        }
        lengths[k] = length;
        totallength += length;
        bottomTypes = function1(A, nx, ny, k+1); // the same as topTypes
    }
    unsigned** out = malloc(4 * sizeof(unsigned*));
    ......
    ......
    return out;
}

As you can see, at the end of the loop, I do bottomTypes = function1(A, nx, ny, k+1); which is the same as topTypes previousy introduced. Thus function1(A,nx,ny,k+1) is called twice. That's because I've not been able to copy topTypes in bottomTypes. The code produced the expected output when I do like this.

Instead of doing like this, to avoid the doube call, I've tried

**bottomTypes = **topTypes;

or

bottomTypes = malloc((nx-1) * sizeof(unsigned*));
for ( int i = 0; i < nx-1; ++i ){
    bottomTypes[i] = malloc((ny-1)*sizeof(unsigned));
    memcpy(bottomTypes[i], topTypes[i], ny-1);
}

The code compiles but I don't get the expected resut when I do that.

What is the right way to copy topTypes into bottomTypes ?

I hope this is clear. Otherwise do not hesitate to say me it's not clear and I'll try to make a reproducibe exampe, but it's not easy. I've found similar questions on SO but none of them allows me to solve my issue.

Edit

Below is a complete reproducibe example. Not very minimal, I admit.

#include <stdlib.h> // for malloc
#include <stdio.h>  // for printf

// function to create a "three-dimensional array" (I know this is not the correct wording)
//  from a given function
double*** fun2array(double f(unsigned,unsigned,unsigned), unsigned nx, unsigned ny, unsigned nz){
    double*** out = (double***) malloc(nx * sizeof(double**));
    for(unsigned i=0; i<nx; i++){
        out[i] = (double**) malloc(ny * sizeof(double*));
        for(unsigned j=0; j<ny; j++){
            out[i][j] = (double*) malloc(nz * sizeof(double));
            for(unsigned k=0; k<nz; k++){
                out[i][j][k] = f(i,j,k);
            }
        }
    }
    return out;
}

// a three-dimensional array
double fun(unsigned i, unsigned j, unsigned k){
    return i+j+k;
}
double*** A = fun2array(fun, 2, 3, 4);

// function to "slice" a 3d-array to a 2D-array (a "matrix")
double** toMatrix(double*** A, unsigned nx, unsigned ny, unsigned k){
    double** out = (double**) malloc(nx * sizeof(double*));
    for(unsigned i=0; i<nx; i++){
        out[i] = (double*) malloc(ny * sizeof(double));
        for(unsigned j=0; j<ny; j++){
            out[i][j] = A[i][j][k];
        }
    }
    return out;    
}

// function to convert double matrix to integer matrix
unsigned** floorMatrix(double** M , unsigned nx, unsigned ny){
    unsigned** out = (unsigned**) malloc(nx * sizeof(unsigned*));
    for(unsigned i=0; i<nx; i++){
        out[i] = (unsigned*) malloc(ny * sizeof(unsigned));
        for(unsigned j=0; j<ny; j++){
            out[i][j] = (unsigned) M[i][j];
        }
    }
    return out;
}

// function to sum 2 "matrices"
unsigned** matricialSum(unsigned** M1, unsigned** M2, unsigned nx, unsigned ny){
    unsigned** out = (unsigned**) malloc(nx * sizeof(unsigned*));
    for(unsigned i=0; i<nx; i++){
        out[i] = (unsigned*) malloc(ny * sizeof(unsigned));
        for(unsigned j=0; j<ny; j++){
            out[i][j] = M1[i][j] + M2[i][j];
        }
    }
    return out;
}

unsigned myfunction(double*** A, unsigned nx, unsigned ny, unsigned nz){
    unsigned** bottomTypes = floorMatrix(toMatrix(A, nx, ny, 0), nx, ny);
    unsigned** cellTypes;
    for(unsigned k=0; k<nz-1; k++){
        unsigned** topTypes = floorMatrix(toMatrix(A, nx, ny, k+1), nx, ny);
        cellTypes = matricialSum(bottomTypes, topTypes, nx, ny);
        bottomTypes = floorMatrix(toMatrix(A, nx, ny, k+1), nx, ny); // the same as topTypes
    }
    return cellTypes[0][0];
}


int main(){
    unsigned result = myfunction(A, 2, 3, 4);
    printf("result:%u\n", result);
    return 0;
}

2nd edit

I have a solution, but surey not optimal:

unsigned** copyMatrix(unsigned** M, unsigned nx, unsigned ny){
    unsigned** MM = malloc(nx * sizeof(unsigned*));
    for(unsigned i=0; i<nx; i++){
        MM[i] = malloc(ny * sizeof(unsigned));
        for(unsigned j=0; j<ny; j++){
            MM[i][j] = M[i][j];
        }
    }
    return MM;
}

Then in the example code I free bottomTypes and then I do

unsigned** bottomTypes = copyMatrix(topTypes, nx-1, ny-1);
Stéphane Laurent
  • 75,186
  • 15
  • 119
  • 225
  • 4
    As a rule of thumb: whenever you have more than 2 levels of indirection in a C program, your design has gone bad. This is called "three star programming" and is a sarcasm for code that is needlessly complex. Perhaps you meant to use [proper multi-dimensional arrays](https://stackoverflow.com/questions/42094465/correctly-allocating-multi-dimensional-arrays) instead? – Lundin Apr 10 '18 at 12:37
  • Excuse-me @Stargateur, what is "NANI" ? `A***` is a three-dimensional array. – Stéphane Laurent Apr 10 '18 at 12:40
  • Thanks for your comment @Lundin, but this sounds too complex for me, sorry. I'm really novice in C. I even don't see what you mean by "2 levels of indirection". – Stéphane Laurent Apr 10 '18 at 12:42
  • http://wiki.c2.com/?ThreeStarProgrammer, it's simple as the number of `*` your variable have, basically, you have a pointer of pointer of pointer of double... that clearly something that should never happen in C. We can't help you a lot cause you didn't give a [mcve] or the context of your code. I clearly see that your data structure could be improve a lot but without have detail we can't help you. – Stargateur Apr 10 '18 at 12:46
  • *pointer = 1 level of indirection, **pointer = 2 levels. And so on. `A***` is _not_ a three-dimensional array. Using three dimensional arrays is much easier than obscure three star programming. Just one malloc, one free. – Lundin Apr 10 '18 at 12:46
  • And yeah I think you need to clarify this with a minimal example, because the whole program design seems bad from the start and could likely be rewritten in much simpler and faster ways. – Lundin Apr 10 '18 at 12:48
  • @Lundin Ok, I will try. But it will take me a certain time. Please guys wait for my edit before downvoting ;-) – Stéphane Laurent Apr 10 '18 at 12:54
  • bottomTypes = topTypes ?? – purec Apr 10 '18 at 13:45
  • @Lundin Done. I've added a complete example (which has nothing to do with my real exampe, I tried to make something minimal). – Stéphane Laurent Apr 10 '18 at 13:48
  • @purec Yes, both are `function1(A, nx, ny, k+1)`. But then `topTypes` is updated at the next step of the loop. – Stéphane Laurent Apr 10 '18 at 13:50
  • @StéphaneLaurent Well, this isn't valid C. You can't initialize a global variable like `double*** A = fun2array(fun, 2, 3, 4);`. – Lundin Apr 10 '18 at 14:09
  • @Lundin ok (however the code works) Can I put `double*** A = fun2array(fun, 2, 3, 4);` in `main` then ? – Stéphane Laurent Apr 10 '18 at 14:19
  • floorMatrix() does it's job. I think there is no easier way. – purec Apr 10 '18 at 14:22
  • I don't understand why you need 3D matrix, sorry these maths are out of my league, I can see a lot of way, to produce better code, but like You didn't provide context of how this code is suppose to be use, I can't help you without be sure to break the code. However here a better readable code version, http://rextester.com/LUOMOF9870. – Stargateur Apr 10 '18 at 14:25
  • @Stargateur This is for the *marching cubes algorithm* A function must be evaluated at each vertex of a 3D-grid. Thanks for the link, I will take a look. – Stéphane Laurent Apr 10 '18 at 14:30
  • I would recommend you switch to a linear array representation for the matrices, similar to what I outlined [here](https://stackoverflow.com/a/34862940/1475978). In your case, instead of `rows`, `cols`, `rowstride`, and `colstride`, you'd have for example `nx`, `ny`, `nz`, `xstride`, `ystride` and `zstride`. Element `(x, y, z)` of `m` is then at `m->origin[x*m->xstride + y*m->ystride + z*m->zstride]`. Its power is in the flexibility it allows in managing the matrices, including matrices that are "views" (transposed and/or partial) to other matrices. – Nominal Animal Apr 10 '18 at 18:06

1 Answers1

2

I finally understand your question, you write **bottomTypes = **topTypes; in place of bottomTypes = topTypes;. This is simply replace pointer by another.

However there are a lot of potential issue with your code, you have memory leak, you use ThreeStar, you cast return of malloc, etc. If your purpose to use C is to be fast, I advice you to change your data structure. malloc() is expensive and unless you want to be able to swap two lines of your matrix with O(1), there are no need to have one malloc for every lines of yours matrix.

So:

  • Do I cast the result of malloc?
  • You could use FAM if you don't want too many malloc()
  • You have memory leak each time you drop topTypes and bottomTypes or cellTypes, I suppose it only in your [mcve].
  • In your orignal code you use VLA, this should be use carefully.
  • You use unsigned instead of size_t for index, not a big problem if you don't use very big matrix.

Your [mcve] with little fix, memory leak everywhere:

#include <stdlib.h> // for malloc
#include <stdio.h>  // for printf

// function to create a "three-dimensional array" (I know this is not the
// correct wording)
//  from a given function
double ***fun2array(double (*f)(size_t, size_t, size_t), size_t nx, size_t ny,
                    size_t nz) {
  double ***out = malloc(nx * sizeof *out);
  for (size_t i = 0; i < nx; i++) {
    out[i] = malloc(ny * sizeof *out[i]);
    for (size_t j = 0; j < ny; j++) {
      out[i][j] = malloc(nz * sizeof *out[i][j]);
      for (size_t k = 0; k < nz; k++) {
        out[i][j][k] = f(i, j, k);
      }
    }
  }
  return out;
}

// a three-dimensional array
double fun(size_t i, size_t j, size_t k) { return i + j + k; }

// function to "slice" a 3d-array to a 2D-array (a "matrix")
double **toMatrix(double ***A, size_t nx, size_t ny, size_t k) {
  double **out = malloc(nx * sizeof *out);
  for (size_t i = 0; i < nx; i++) {
    out[i] = malloc(ny * sizeof *out[i]);
    for (size_t j = 0; j < ny; j++) {
      out[i][j] = A[i][j][k];
    }
  }
  return out;
}

// function to convert double matrix to integer matrix
unsigned **floorMatrix(double **M, size_t nx, size_t ny) {
  unsigned **out = malloc(nx * sizeof *out);
  for (size_t i = 0; i < nx; i++) {
    out[i] = malloc(ny * sizeof *out[i]);
    for (size_t j = 0; j < ny; j++) {
      out[i][j] = M[i][j];
    }
  }
  return out;
}

// function to sum 2 "matrices"
unsigned **matricialSum(unsigned **M1, unsigned **M2, size_t nx, size_t ny) {
  unsigned **out = malloc(nx * sizeof *out);
  for (size_t i = 0; i < nx; i++) {
    out[i] = malloc(ny * sizeof *out[i]);
    for (size_t j = 0; j < ny; j++) {
      out[i][j] = M1[i][j] + M2[i][j];
    }
  }
  return out;
}

unsigned myfunction(double ***A, size_t nx, size_t ny, size_t nz) {
  unsigned **bottomTypes = floorMatrix(toMatrix(A, nx, ny, 0), nx, ny);
  unsigned **cellTypes = bottomTypes;
  for (size_t k = 1; k < nz; k++) {
    unsigned **topTypes = floorMatrix(toMatrix(A, nx, ny, k), nx, ny);
    cellTypes = matricialSum(bottomTypes, topTypes, nx, ny);
    bottomTypes = topTypes;
  }
  return cellTypes[0][0];
}

int main(void) {
  double ***A = fun2array(fun, 2, 3, 4);
  unsigned result = myfunction(A, 2, 3, 4);
  printf("result:%u\n", result);
}
Stargateur
  • 24,473
  • 8
  • 65
  • 91
  • 1
    Thanks a lot! I will study all that carefully. So I can simpy do `bottomTypes = topTypes` ? That's weird, I was sure I tried that and there was an issue. But maybe I did something else wrong. Thanks again ! – Stéphane Laurent Apr 10 '18 at 15:12