Matrix Multiplication & Addition

Question

I created this small library to perform basic arithmetic on matrices. I have written the code, and it runs fine. Is there any unexpected behavior or memory leaks? Any room for improvement?

#include <stdio.h>
#include <stdlib.h>
typedef struct {
 int m; // rows
 int n; // columns
 int* elements;
} matrix_t;
matrix_t* create_matrix(int m, int n) {
 matrix_t* new_matrix = malloc(sizeof(matrix_t));
 new_matrix->m = m;
 new_matrix->n = n;
 new_matrix->elements = malloc(sizeof(int) * m * n);
 for (int i = 0; i < m * n; i++)
 new_matrix->elements[i] = 0;
 return new_matrix;
}
matrix_t* multiply_matrix(matrix_t** first, matrix_t** second) {
 if (first == NULL || second == NULL) return NULL;
 if ((*first)->n != (*second)->m) return NULL;
 matrix_t* product = create_matrix((*first)->m, (*second)->n);
 for (int i = 0; i < (*first)->m; i++) {
 for (int j = 0; j < (*second)->n; j++) {
 for (int k = 0; k < (*second)->m; k++)
 product->elements[j + i * product->n] +=
 (*first)->elements[k + i * (*first)->n] * (*second)->elements[j + k * (*second)->n];
 }
 }
 return product;
}
matrix_t* add_matrix(matrix_t** first, matrix_t** second) {
 if (first == NULL || second == NULL) return NULL;
 if ((*first)->m != (*second)->m && (*first)->n != (*second)->n) return NULL;
 matrix_t* sum = create_matrix((*first)->m, (*first)->n);
 for (int i = 0; i < sum->n; i++)
 for (int j = 0; j < sum->m; j++)
 sum->elements[j + i * sum->n] =
 (*first)->elements[j + i * (*first)->n] + (*second)->elements[j + i * (*second)->n];
 return sum;
}
void print_matrix(matrix_t** mat) {
 if (mat == NULL) return;
 for (int i = 0; i < (*mat)->m; i++) {
 for (int j = 0; j < (*mat)->n; j++)
 printf("%5d", (*mat)->elements[j + i * (*mat)->n]);
 puts("");
 }
}

• chux: "basic arithmetic on matrices. " --> Interesting that OP 's element type is an int. I'd expect a double.
• chux: nabat, Why ultiply_matrix(matrix_t** first, matrix_t** second) instead of ultiply_matrix(matrix_t* first, matrix_t* second)?

Answer 1

Other than what vnp already said in their answer, I'd like to point out a logic error in the code. When checking whether the matrices have the same dimensions in add_matrix, you use the following condition:

(*first)->m != (*second)->m && (*first)->n != (*second)->n

However, there's a problem here. Say you pass a matrix with dimensions m=2, n=3 and another with dimensions m=2, n=5. Since they both have the same m, the first part of the condition will evaluate to false and the && will short circuit to false without ever comparing the ns, which are different. This can be fixed by replacing the && with an ||, as you need at least one of the matrices' dimensions to not match. Hence, the correct condition is:

(*first)->m != (*second)->m || (*first)->n != (*second)->n

Answer 2

• create_matrix happily accepts negative arguments. It does not exactly make sense. Semantically m and n (both as arguments and as structure members) should be size_t.

  As a side note, prefer calloc to a manual zero-filling.

• Why double indirection? Passing first and second as matrix_t * makes the code much more readable.

  While we are here, neither add nor multiply mutate the arguments. Qualify them const.

• nabat: Thanks for the feedback. I've done as you said and made the width and height of size_t. I also used double indirection because I was getting pointer errors but now I don't know what the problem was. I've also made the arguments constant, but I don't understand the calloc part, why does that work?
• vnp: @PasserBy You'd get a compile-time warning, wouldn't you?
• Andrew Henle: As a side note, prefer calloc to a manual zero-filling. Using calloc() instead of malloc() and then memset() does have one potential performance-impacting quirk - the virtual memory pages might not actually be mapped until they're accessed. That could cause quite a performance hit during computation if a large array continually causes stalls as each page gets mapped one-by-one the first time it's used. That might cause a problem because of when the latency appears.
• chux: @AndrewHenle Some memory systems use idle time (or another processor) to zero fill blocks of memory - to be on hand for a calloc() like call.
• Andrew Henle: @chux-ReinstateMonica See stackoverflow.com/questions/2688466/… Or maybe sift through all of these: google.com/search?q=calloc+lazy+mapping+site:stackexchange.com An application might not want to have to wait for a bunch of lazy-mapped-memory pages to get mapped in the middle of a performance-critical operation. Calling memset() when allocating the memory is one way to do that. Imagine a high-volume data stream that can deliver data faster than pages can be mapped. Ooops - that data now gets lost. BTDT.

Answer 3

This code is completely missing the error-handling for when malloc() returns a null pointer. Which makes the entire program's behaviour undefined on any occasion we run out of storage.

---

This is suspect:

matrix_t* multiply_matrix(matrix_t** first, matrix_t** second) {
 if (first == NULL || second == NULL) return NULL;
 if ((*first)->n != (*second)->m) return NULL;

We don't check *first or *second are not null before dereferencing them. Mind you, it's unclear why we're passing pointer to pointer here.

---

I don't think these functions have any business modifying the matrices passed as arguments. So they should be matrix_t const* const* to express (and enforce) that.

---

The calculation of matrix->elements[column + row * matrix->n] is repeated several times; I think it would be clearer as a small inlinable function (or perhaps a pair of functions for const-correct read and write).

---

If we're always going to dynamically allocate matrix_t objects, then perhaps we should use a flexible array member for elements rather than another pointer indirection.