patterncMinor
C struct or pointer to struct?
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...
-
-
All ideas are welcome!
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:
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:
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:
However it's bad practice: if you're coding in C, you should become familiar with using pointers.
There's nothing wrong with the
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 *mpointer) 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.