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.

I am currently using this code (and more) to deal with 2D matrices. I am not very proficient in C, so I am not quite sure, if this is a good way of doing it. I've seen other people in similar situations using pointers to a struct, but I didn't want to complicate things...

typedef struct CMatrix {
    double *m;
    uint32_t width;
    uint32_t height;
} CMatrix;

-

CMatrix newCMatrix(uint32_t width, uint32_t height) {
    CMatrix rtn;
    rtn.m = malloc(width * height * sizeof(double));
    bzero(rtn.m, width * height * sizeof(double));

    rtn.width = width;
    rtn.height = height;

    return rtn;
}

-

CMatrix ma = newCMatrix(2, 2);

All ideas are welcome!

share|improve this question
    
What is the use of this bzero ? –  Midhun MP Mar 14 at 19:39
    
because the memory returned by malloc() is not initialized, I'm setting it to all zeros. –  JoJoe Mar 14 at 19:45
    
Use memset instead of bzero. But it is better to use calloc than using malloc + memset –  Midhun MP Mar 14 at 19:51
    
You are not using signed integers to represent sizes/indices (Well done), but note that the standard library provide an specific type for this purpose: size_t –  Manu343726 Mar 15 at 0:45

2 Answers 2

up vote 3 down vote accepted

Pointers seem more complicated, but they're usually faster and more correct.

Faster

For example, perhaps you have a function like this:

CMatrix add(CMatrix left, CMatrix right)
{
     ... add the matrices ...
}

In the above you're passing matrix parameters by value instead of by pointer. That means that you're passing a copy of each matrix (instead of a pointer to each matrix).

Assuming you have 32-bit pointers, each matrix is 12 bytes in size (4 bytes for the pointer and 4 bytes for each integer), so you're passing 24 bytes (for two matrices) instead of 8 bytes (for two pointers-to-matrices).

That (24 bytes instead of 8 bytes) is not a big difference (i.e. not much slower); but it's a bigger difference when your structs are larger and more complicated.

More correct

Another problem is that subroutines are operating on a private, local copy of the matrix; for example:

void transpose(CMatrix matrix)
{
    ... height becomes width and vice versa ...
    uint32_t oldWidth = matrix.width;
    uint32_t oldHeight = matrix.height;
    matrix.width = oldHeight;
    matrix.height = oldWidth;
    ... and transpose the data ...
}

This function changes the width and height of its local copy of the matrix, not the width and height of the caller's copy of the matrix.

Summary

Your code happens to work because:

  • You have a simple structure
  • You don't alter height and width in a subroutine, after you create the matrix
  • The data itself within the matrix (i.e. the double *m pointer) is a pointer (and is therefore shared between subroutines)

However it's bad practice: if you're coding in C, you should become familiar with using pointers.

There's nothing wrong with the newCMatrix function you showed in the OP. What I'm criticizing is your statement that pointers complicate things, because they're usually necessary, e.g. for other functions; for example, the 'add' function above should (in order to be idiomatic) be declared more like the following:

CMatrix add(const CMatrix *left, const CMatrix *right)
{
     ... add the matrices ...
}
share|improve this answer
    
Presumably, transpose(const CMatrix) would return the result in a new CMatrix. The caller would have to remember to free a lot of stuff, which could get tricky. –  200_success Mar 14 at 21:07
    
Yes, transpose would have to return a new CMatrix if it couldn't alter the existing one: CMatrix transposed = transpose(matrix);. Perhaps pass-by-value works well for the OP, given the peculiar nature of the CMatrix struct (i.e. logically const height and width properties, with the mutable data located in a pointer-type member); I hoped to warn that C code doesn't always/generally work correctly without using pointers, and that pointers are idiomatic. –  ChrisW Mar 14 at 21:19
    
Thanks! I didn't even think about the whole call by value / call by reference issue, because so far I didn't have any functions that manipulate the matrices... –  JoJoe Mar 14 at 22:07

This answer replaces poor advice in a previous answer.

You can use calloc() instead of malloc() and bzero().

Your newCMatrix() is otherwise fine. If you write a newCMatrix(uint32_t, uint32_t), though, you should also provide a corresponding freeCMatrix(CMatrix) to deallocate the array.

share|improve this answer
    
Thanks! I actually have a freeCMatrix() function, but I didn't include it in this post. –  JoJoe Mar 14 at 20:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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