HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

C struct or pointer to struct?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pointerstructstackoverflow

Problem

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!

Solution

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 ...
}

Code Snippets

CMatrix add(CMatrix left, CMatrix right)
{
     ... add the matrices ...
}
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 ...
}
CMatrix add(const CMatrix *left, const CMatrix *right)
{
     ... add the matrices ...
}

Context

StackExchange Code Review Q#44387, answer score: 4

Revisions (0)

No revisions yet.