10
\$\begingroup\$

I implemented a two-dimensional array in C and would like to know if the implementation is sound in terms of

  1. Memory management

  2. Potential bugs and other side effects that I missed

  3. Code style (readability, naming conventions, etc...)

  4. Clean code

  5. Efficiency

The code compiles and runs without any error: gcc -O -Wall main.c

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

struct Array {
    double **data;
    size_t rows;
    size_t cols;
};

struct Array *NewArray(const size_t rows, const size_t cols){
    if (rows <= 0 || cols <= 0){
        return NULL;
    }
    struct Array *array = (struct Array *)malloc(sizeof(struct Array *));

    array->rows = rows;
    array->cols = cols;

    array->data = (double **) malloc(rows * sizeof(double *));
    for (size_t i = 0; i < rows; ++i) {
        array->data[i] = (double *) malloc(cols * sizeof(double));
    }

    return array;
}

int FreeArray(struct Array *array){
    if (array != NULL) {
        return -1;
    }

    assert (array->data);
    for (size_t i = 0; i < array->rows; ++i) {
        free(array->data[i]);
    }
    free(array->data);
    free(array);
    return 0;
}

void PrintArray(const struct Array *array) {
    for (int i=0; i<array->rows; ++i) {
        for (int j=0; j<array->cols; ++j) {
            printf("%.2lf ", array->data[i][j]);
        }
        printf("\n");
    }
    return;
}

int main(){
    size_t rows = 2;
    size_t cols = 4;
    struct Array *M = NewArray(2, 4);
    PrintArray(M);
    FreeArray(M);
}
\$\endgroup\$
2
  • 5
    \$\begingroup\$ It’s unfortunate that every beginner to C learns char** argv right away. That makes them think that’s the right data structure to use for “a two-dimensional array,” and it almost never is. Worse is when they think the way to code a three-dimensional array is with three stars. \$\endgroup\$
    – Davislor
    Commented Dec 14, 2022 at 14:32
  • 1
    \$\begingroup\$ There is a misunderstanding being propagated in answers that using double **data; vs. double *data; implies many vs. few allocations. With double **data; only 1 allocation is required. Using 1 allocation or many (like 1 per row) is best depends on how the larger code is going to use struct Array. \$\endgroup\$
    – chux - Reinstate Monica
    Commented Dec 15, 2022 at 9:45

5 Answers 5

Reset to default
14
\$\begingroup\$

Enable more warnings, and use a memory checker:

gcc-12 -std=c17 -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion  -Wstrict-prototypes -fanalyzer 281916.c -o 281916
281916.c: In function ‘PrintArray’:
281916.c:43:20: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
   43 |     for (int i=0; i<array->rows; ++i) {
      |                    ^
281916.c:44:24: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
   44 |         for (int j=0; j<array->cols; ++j) {
      |                        ^
281916.c: At top level:
281916.c:52:5: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
   52 | int main(){
      |     ^~~~
281916.c: In function ‘main’:
281916.c:54:12: warning: unused variable ‘cols’ [-Wunused-variable]
   54 |     size_t cols = 4;
      |            ^~~~
281916.c:53:12: warning: unused variable ‘rows’ [-Wunused-variable]
   53 |     size_t rows = 2;
      |            ^~~~
281916.c: In function ‘NewArray’:
281916.c:17:17: warning: dereference of possibly-NULL ‘array’ [CWE-690] [-Wanalyzer-possible-null-dereference]
   17 |     array->rows = rows;
      |     ~~~~~~~~~~~~^~~~~~
  ‘NewArray’: events 1-4
    |
    |   12 |     if (rows <= 0 || cols <= 0){
    |      |        ^
    |      |        |
    |      |        (1) following ‘false’ branch...
    |......
    |   15 |     struct Array *array = (struct Array *)malloc(sizeof(struct Array *));
    |      |                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                                           |
    |      |                                           (2) ...to here
    |      |                                           (3) this call could return NULL
    |   16 | 
    |   17 |     array->rows = rows;
    |      |     ~~~~~~~~~~~~~~~~~~
    |      |                 |
    |      |                 (4) ‘array’ could be NULL: unchecked value from (3)
    |
281916.c: In function ‘PrintArray’:
281916.c:45:13: warning: use of uninitialized value ‘*_5 + _7’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
   45 |             printf("%.2lf ", array->data[i][j]);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-2
    |
    |   52 | int main(){
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |......
    |   55 |     struct Array *M = NewArray(2, 4);
    |      |                       ~~~~~~~~~~~~~~
    |      |                       |
    |      |                       (2) calling ‘NewArray’ from ‘main’
    |
    +--> ‘NewArray’: events 3-12
           |
           |   11 | struct Array *NewArray(const size_t rows, const size_t cols){
           |      |               ^~~~~~~~
           |      |               |
           |      |               (3) entry to ‘NewArray’
           |   12 |     if (rows <= 0 || cols <= 0){
           |      |        ~       
           |      |        |
           |      |        (4) following ‘false’ branch...
           |......
           |   15 |     struct Array *array = (struct Array *)malloc(sizeof(struct Array *));
           |      |                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                                           |
           |      |                                           (5) ...to here
           |......
           |   21 |     for (size_t i = 0; i < rows; ++i) {
           |      |                        ~~~~~~~~
           |      |                          |
           |      |                          (6) following ‘true’ branch (when ‘i < rows’)...
           |      |                          (9) following ‘true’ branch (when ‘i < rows’)...
           |      |                          (11) following ‘false’ branch (when ‘i >= rows’)...
           |   22 |         array->data[i] = (double *) malloc(cols * sizeof(double));
           |      |                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                                     |
           |      |                                     (7) ...to here
           |      |                                     (8) region created on heap here
           |      |                                     (10) ...to here
           |......
           |   25 |     return array;
           |      |            ~~~~~
           |      |            |
           |      |            (12) ...to here
           |
    <------+
    |
  ‘main’: events 13-14
    |
    |   55 |     struct Array *M = NewArray(2, 4);
    |      |                       ^~~~~~~~~~~~~~
    |      |                       |
    |      |                       (13) returning to ‘main’ from ‘NewArray’
    |   56 |     PrintArray(M);
    |      |     ~~~~~~~~~~~~~      
    |      |     |
    |      |     (14) calling ‘PrintArray’ from ‘main’
    |
    +--> ‘PrintArray’: events 15-20
           |
           |   42 | void PrintArray(const struct Array *array) {
           |      |      ^~~~~~~~~~
           |      |      |
           |      |      (15) entry to ‘PrintArray’
           |   43 |     for (int i=0; i<array->rows; ++i) {
           |      |                   ~~~~~~~~~~~~~
           |      |                    |
           |      |                    (16) following ‘true’ branch...
           |   44 |         for (int j=0; j<array->cols; ++j) {
           |      |                  ~    ~~~~~~~~~~~~~
           |      |                  |     |
           |      |                  |     (18) following ‘true’ branch...
           |      |                  (17) ...to here
           |   45 |             printf("%.2lf ", array->data[i][j]);
           |      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |             |                     |
           |      |             |                     (19) ...to here
           |      |             (20) use of uninitialized value ‘*_5 + _7’ here
           |
281916.c:45:13: warning: use of uninitialized value ‘*_5 + _7’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
   45 |             printf("%.2lf ", array->data[i][j]);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-2
    |
    |   52 | int main(){
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |......
    |   55 |     struct Array *M = NewArray(2, 4);
    |      |                       ~~~~~~~~~~~~~~
    |      |                       |
    |      |                       (2) calling ‘NewArray’ from ‘main’
    |
    +--> ‘NewArray’: events 3-12
           |
           |   11 | struct Array *NewArray(const size_t rows, const size_t cols){
           |      |               ^~~~~~~~
           |      |               |
           |      |               (3) entry to ‘NewArray’
           |   12 |     if (rows <= 0 || cols <= 0){
           |      |        ~       
           |      |        |
           |      |        (4) following ‘false’ branch...
           |......
           |   15 |     struct Array *array = (struct Array *)malloc(sizeof(struct Array *));
           |      |                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                                           |
           |      |                                           (5) ...to here
           |......
           |   21 |     for (size_t i = 0; i < rows; ++i) {
           |      |                        ~~~~~~~~
           |      |                          |
           |      |                          (6) following ‘true’ branch (when ‘i < rows’)...
           |      |                          (9) following ‘true’ branch (when ‘i < rows’)...
           |      |                          (11) following ‘false’ branch (when ‘i >= rows’)...
           |   22 |         array->data[i] = (double *) malloc(cols * sizeof(double));
           |      |                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                                     |
           |      |                                     (7) ...to here
           |      |                                     (8) region created on heap here
           |      |                                     (10) ...to here
           |......
           |   25 |     return array;
           |      |            ~~~~~
           |      |            |
           |      |            (12) ...to here
           |
    <------+
    |
  ‘main’: events 13-14
    |
    |   55 |     struct Array *M = NewArray(2, 4);
    |      |                       ^~~~~~~~~~~~~~
    |      |                       |
    |      |                       (13) returning to ‘main’ from ‘NewArray’
    |   56 |     PrintArray(M);
    |      |     ~~~~~~~~~~~~~      
    |      |     |
    |      |     (14) calling ‘PrintArray’ from ‘main’
    |
    +--> ‘PrintArray’: events 15-22
           |
           |   42 | void PrintArray(const struct Array *array) {
           |      |      ^~~~~~~~~~
           |      |      |
           |      |      (15) entry to ‘PrintArray’
           |   43 |     for (int i=0; i<array->rows; ++i) {
           |      |                   ~~~~~~~~~~~~~
           |      |                    |
           |      |                    (16) following ‘true’ branch...
           |   44 |         for (int j=0; j<array->cols; ++j) {
           |      |                  ~    ~~~~~~~~~~~~~
           |      |                  |     |
           |      |                  |     (18) following ‘true’ branch...
           |      |                  |     (20) following ‘true’ branch...
           |      |                  (17) ...to here
           |   45 |             printf("%.2lf ", array->data[i][j]);
           |      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |             |                     |
           |      |             |                     (19) ...to here
           |      |             |                     (21) ...to here
           |      |             (22) use of uninitialized value ‘*_5 + _7’ here
           |
make[2]: Leaving directory '/home/tms/stackexchange/review'
valgrind --leak-check=full ./281916 ==4108801== Memcheck, a memory error detector
==4108801== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==4108801== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==4108801== Command: ./281916
==4108801== 
==4108801== Invalid write of size 8
==4108801==    at 0x1091B8: NewArray (281916.c:17)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801==  Address 0x4a5a048 is 0 bytes after a block of size 8 alloc'd
==4108801==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4108801==    by 0x1091AB: NewArray (281916.c:15)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801== 
==4108801== Invalid write of size 8
==4108801==    at 0x1091C4: NewArray (281916.c:18)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801==  Address 0x4a5a050 is 8 bytes after a block of size 8 alloc'd
==4108801==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4108801==    by 0x1091AB: NewArray (281916.c:15)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801== 
==4108801== Invalid read of size 8
==4108801==    at 0x109365: PrintArray (281916.c:43)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801==  Address 0x4a5a048 is 0 bytes after a block of size 8 alloc'd
==4108801==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4108801==    by 0x1091AB: NewArray (281916.c:15)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801== 
==4108801== Invalid read of size 8
==4108801==    at 0x109344: PrintArray (281916.c:44)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801==  Address 0x4a5a050 is 8 bytes after a block of size 8 alloc'd
==4108801==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4108801==    by 0x1091AB: NewArray (281916.c:15)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C8FC8: __printf_fp_l (printf_fp.c:396)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C8FE2: __printf_fp_l (printf_fp.c:396)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C4112: __mpn_extract_double (dbl2mpn.c:56)
==4108801==    by 0x48C958E: __printf_fp_l (printf_fp.c:396)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C4117: __mpn_extract_double (dbl2mpn.c:60)
==4108801==    by 0x48C958E: __printf_fp_l (printf_fp.c:396)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C9897: __printf_fp_l (printf_fp.c:978)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C98D5: __printf_fp_l (printf_fp.c:981)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48CAA78: __printf_fp_l (printf_fp.c:991)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C9926: round_away (rounding-mode.h:52)
==4108801==    by 0x48C9926: __printf_fp_l (printf_fp.c:998)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C9B94: __printf_fp_l (printf_fp.c:1166)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C9F7E: __printf_fp_l (printf_fp.c:1228)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48C9F63: __printf_fp_l (printf_fp.c:1230)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48F81D9: _IO_file_overflow@@GLIBC_2.2.5 (fileops.c:782)
==4108801==    by 0x48CA047: __printf_fp_l (printf_fp.c:1254)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48D39E4: __vfprintf_internal (vfprintf-internal.c:1062)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48D39F4: done_add_func (vfprintf-internal.c:127)
==4108801==    by 0x48D39F4: __vfprintf_internal (vfprintf-internal.c:1067)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48D39FC: __vfprintf_internal (vfprintf-internal.c:1067)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48D2897: done_add_func (vfprintf-internal.c:127)
==4108801==    by 0x48D2897: outstring_func (vfprintf-internal.c:241)
==4108801==    by 0x48D2897: __vfprintf_internal (vfprintf-internal.c:1096)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48D28B0: done_add_func (vfprintf-internal.c:127)
==4108801==    by 0x48D28B0: outstring_func (vfprintf-internal.c:241)
==4108801==    by 0x48D28B0: __vfprintf_internal (vfprintf-internal.c:1096)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Conditional jump or move depends on uninitialised value(s)
==4108801==    at 0x48D28B8: __vfprintf_internal (vfprintf-internal.c:1096)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
==4108801== Syscall param write(buf) points to uninitialised byte(s)
==4108801==    at 0x496E190: write (write.c:26)
==4108801==    by 0x48F6E64: _IO_file_write@@GLIBC_2.2.5 (fileops.c:1180)
==4108801==    by 0x48F621F: new_do_write (fileops.c:448)
==4108801==    by 0x48F7E78: _IO_do_write@@GLIBC_2.2.5 (fileops.c:425)
==4108801==    by 0x48F8282: _IO_file_overflow@@GLIBC_2.2.5 (fileops.c:783)
==4108801==    by 0x48EF1B0: putchar (putchar.c:28)
==4108801==    by 0x109356: PrintArray (281916.c:47)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801==  Address 0x4a5a1a2 is 2 bytes inside a block of size 1,024 alloc'd
==4108801==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4108801==    by 0x48EB76B: _IO_file_doallocate (filedoalloc.c:101)
==4108801==    by 0x48F8F4F: _IO_doallocbuf (genops.c:347)
==4108801==    by 0x48F8F4F: _IO_doallocbuf (genops.c:342)
==4108801==    by 0x48F8317: _IO_file_overflow@@GLIBC_2.2.5 (fileops.c:744)
==4108801==    by 0x48CA047: __printf_fp_l (printf_fp.c:1254)
==4108801==    by 0x48D3EEC: __printf_fp_spec (vfprintf-internal.c:354)
==4108801==    by 0x48D3EEC: __vfprintf_internal (vfprintf-internal.c:1061)
==4108801==    by 0x48C84FA: printf (printf.c:33)
==4108801==    by 0x109335: PrintArray (281916.c:45)
==4108801==    by 0x1093AB: main (281916.c:56)
==4108801== 
0.00 0.00 0.00 0.00 
0.00 0.00 0.00 0.00 
==4108801== 
==4108801== HEAP SUMMARY:
==4108801==     in use at exit: 88 bytes in 4 blocks
==4108801==   total heap usage: 5 allocs, 1 frees, 1,112 bytes allocated
==4108801== 
==4108801== 88 (8 direct, 80 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==4108801==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4108801==    by 0x1091AB: NewArray (281916.c:15)
==4108801==    by 0x10939B: main (281916.c:55)
==4108801== 
==4108801== LEAK SUMMARY:
==4108801==    definitely lost: 8 bytes in 1 blocks
==4108801==    indirectly lost: 80 bytes in 3 blocks
==4108801==      possibly lost: 0 bytes in 0 blocks
==4108801==    still reachable: 0 bytes in 0 blocks
==4108801==         suppressed: 0 bytes in 0 blocks
==4108801== 
==4108801== Use --track-origins=yes to see where uninitialised values come from
==4108801== For lists of detected and suppressed errors, rerun with: -s
==4108801== ERROR SUMMARY: 194 errors from 24 contexts (suppressed: 0 from 0)

You're missing the most basic of checks, using pointers returned from malloc() when they may be null. You need to fix that immediately.

malloc() returns a void*, which is freely convertible to any kind of object pointer without a cast. Adding a cast serves no purpose, except to clutter the code and sometimes to mask a failure to include <stdlib.h>.

It's easier for reviewers if we can see that the size allocated matches the type of pointer without having to go back to the pointer's definition. Consider these:

    struct Array *array = malloc(sizeof *array);

    array->data = malloc(rows * sizeof *array->data);

        array->data[i] = malloc(cols * sizeof *array->data[i]);

See how they are much more obviously correct, without needing access to the rest of the code? (That's probably why you didn't spot the error in array = (struct Array *)malloc(sizeof(struct Array *))).

FreeArray() looks totally broken:

    if (array != NULL) {
        return -1;
    }
    assert (array->data);

The assert() is reached only if array is null, when dereferencing it in array->data is Undefined Behaviour.

The right thing to do is to accept and ignore a null pointer argument, just like free() does.

A popular naming convention is to prefix all the operations with the type name. That means they are predictable, and they fall together alphabetically when listing all symbols

Fixing the above problems gives:

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

struct Array {
    double **data;
    size_t rows;
    size_t cols;
};

struct Array *array_calloc(const size_t rows, const size_t cols)
{
    struct Array *array = malloc(sizeof *array);
    if (!array) {
        return array;
    }

    array->rows = rows;
    array->cols = cols;

    array->data = malloc(rows * sizeof *array->data);
    if (!array->data) {
        free(array);
        return NULL;
    }

    for (size_t i = 0; i < rows; ++i) {
        array->data[i] = malloc(cols * sizeof *array->data[i]);
        if (!array->data[i]) {
            while (i-->0) {
                free(array->data[i]);
            }
            free(array->data);
            free(array);
            return NULL;
        }
        for (size_t j = 0;  j < cols;  ++j) {
            array->data[i][j] = 0.0;
        }
    }

    return array;
}

void array_free(struct Array *array)
{
    if (array) {
        assert (array->data);
        for (size_t i = 0; i < array->rows; ++i) {
            free(array->data[i]);
        }
        free(array->data);
        free(array);
    }
}

void array_print(const struct Array *array)
{
    for (size_t i = 0;  i < array->rows;  ++i) {
        for (size_t j = 0;  j < array->cols;  ++j) {
            printf("%.2f ", array->data[i][j]);
        }
        printf("\n");
    }
    return;
}

#include <errno.h>
#include <string.h>
int main(void)
{
    struct Array *a = array_calloc(2, 4);
    if (!a) {
        fputs(strerror(ENOMEM), stderr);
        return EXIT_FAILURE;
    }
    array_print(a);
    array_free(a);
}

Reconsider the representation. Using a separate allocation for each row can mean that your data are spread all over the address space, which isn't very cache friendly. Consider a single linear array containing all the columns:

struct Array {
    double *data;
    size_t rows;
    size_t cols;
};

static double *array_element(struct Array *a, size_t row, size_t col)
{
   return a->data + row * a->cols + col;
}

This representation also makes it much easier to clean up when one of the allocations fails.

#include <stdio.h>
#include <stdlib.h>

struct Array {
    double *data;
    size_t rows;
    size_t cols;
};

/* private helper */
static double *array_element(const struct Array *a, size_t row, size_t col)
{
   return a->data + row * a->cols + col;
}

/* public interface */
double array_value(const struct Array *a, size_t row, size_t col)
{
    return *array_element(a, row, col);
}

void array_set_value(const struct Array *a, size_t row, size_t col, double v)
{
    *array_element(a, row, col) = v;
}

struct Array *array_new(const size_t rows, const size_t cols)
{
    struct Array *array = malloc(sizeof *array);
    if (!array) {
        return array;
    }

    array->data = malloc(sizeof *array->data * rows * cols);
    if (!array->data) {
        free(array);
        return NULL;
    }

    array->rows = rows;
    array->cols = cols;
    for (size_t j = 0;  j < rows * cols;  ++j) {
        array->data[j] = 0.0;
    }

    return array;
}

void array_free(struct Array *array)
{
    if (array) {
        free(array->data);
        free(array);
    }
}

void array_print(const struct Array *array)
{
    for (size_t i = 0;  i < array->rows;  ++i) {
        for (size_t j = 0;  j < array->cols;  ++j) {
            printf("%.2f ", array_value(array, i, j));
        }
        printf("\n");
    }
}


#include <errno.h>
#include <string.h>
int main(void)
{
    struct Array *a = array_calloc(2, 4);
    if (!a) {
        fputs(strerror(ENOMEM), stderr);
        return EXIT_FAILURE;
    }
    array_print(a);
    array_free(a);
}

If you're really attached to using indexing rather than a function to access array elements, then (assuming that switching to C++ isn't an option!) we can add an array of row pointers into data. To maintain locality of reference, it's best if we can allocate the storage and pointers together, like this:

struct Array {
    double *data;
    double **rows;
    size_t height;
    size_t width;
};

    array->data = malloc(sizeof *array->data * height * width
                         + sizeof *array->rows * height);
    if (!array->data) {
        free(array);
        return NULL;
    }
    array->rows = (double **)((char*)array->data + sizeof *array->data * height * width);
    array->height = height;
    array->width = width;

    for (size_t i = 0;  i < height;  ++i) {
        array->rows[i] = array->data + i * width;
    }

No changes required to any other functions.

Then we can access elements using the accessor functions or using indexing as array->rows[i][j].

\$\endgroup\$
10
  • 1
    \$\begingroup\$ Thank you for this very useful answer. I wonder if there is a way to express array_value(array, i, j) in a more compact way (ideally array[i][j]). As I perform many local sums over neighbor cells in the array, using many array_value(array, i, j) can get confusing pretty quickly. \$\endgroup\$
    – Gilfoyle
    Commented Dec 14, 2022 at 7:58
  • 1
    \$\begingroup\$ I've updated showing how you can do that. \$\endgroup\$
    – Toby Speight
    Commented Dec 14, 2022 at 8:37
  • 2
    \$\begingroup\$ array->rows = (double **)((char*)array->data + sizeof *array->data * height * width); is a problem when the computed address is not aligned for (double **). Consider height, weight are odd and double ** needs a wider than a sizeof(double) alignment. Certainly not likely, yet possible. (I ginned up a single allocate example). \$\endgroup\$
    – chux - Reinstate Monica
    Commented Dec 15, 2022 at 2:55
  • 2
    \$\begingroup\$ @Gilfoyle: Note that Toby's double **rows; member isn't free in terms of performance. It's unlikely for the compiler could prove that array->rows[i][j] is the same as array->data[i*width + j], or that it would even attempt that, so you will be getting an extra level of indirection. Hardware prefetch can still be effective, but you will get extra load latency. Maybe not painful if looping over one row, but if striding down a column, it adds insult to injury: extra latency to load a row pointer before the likely cache miss on the main data can even start. Use C++ if you want [] overload \$\endgroup\$
    – Peter Cordes
    Commented Dec 15, 2022 at 20:58
  • 1
    \$\begingroup\$ That's to match the sizeof that works in units of char (remember that array->data points to double*, which is likely much bigger than char, so without that cast, we'd be adding much more. \$\endgroup\$
    – Toby Speight
    Commented Dec 19, 2022 at 11:11
15
\$\begingroup\$

You are doing too many allocations. The usual way of implementing multidimensional array is to allocate one continuous buffer and map n-dimensional coordinate to a 1-dimensional one. You then provide get and set functions so that client doesn't have to do the mapping themselves. The functions are pretty simple and you can make them inline.

double ArrayGet(const struct Array * array, size_t x, size_t y) {
    return *(array->data + x + y * array->cols);
}

Additionally, you can avoid allocating the struct itself on heap and put it on stack instead. For this you need 2 more functions, but you can reuse them in ArrayNew and ArrayFree.

int main()
{
    struct Array array;
    ArrayInit(&array, 2, 4);

    // write some values

    ArrayPrint(&array);
    ArrayRelease(&array);
}

If you want to go even further and you know the dimensions at compile time, you can put the data buffer on stack as well and just use its address, so avoiding dynamic memory allocation entirely.

int main()
{
  double data[8];
  struct Array array;
  array.rows = 2;
  array.cols = 4;
  array.data = data;

  // write some values

  ArrayPrint(&array);
}

Also notice that you can traverse all elements of the matrix using (x, y) coordinate while internally traversing the one dimensional data buffer in order which is friendly to cached memory access. And you also avoid the few extra instructions for mapping the coordinates. Just make sure your for loops start with y and track the 1-dimensional coordinate i separately.

void ArrayPrint(struct Array * array)
{
    size_t i;
    // using size_t as everywhere else...
    for (size_t y = 0, i = 0; y < array->rows; ++y) {
        for (size_t x = 0; x < array->cols; ++x, ++i) {
            printf("%.2lf ", array->data[i]);
        }
        printf("\n");
    }

    // no return is needed on the end of void function
}
\$\endgroup\$
5
  • 3
    \$\begingroup\$ Another alternative is to use a flexible array member for the data array (which means the Array struct itself must always be in dynamic memory, of course). \$\endgroup\$
    – Toby Speight
    Commented Dec 13, 2022 at 20:53
  • 1
    \$\begingroup\$ Thank you for the insightful answer. The dimensions do not change once the code is compiled. How do I correctly put the data buffer on stack then? As data can be very large, I still need to allocate it on the heap, right? Is it possible to modify your ArrayPrint such that I can do something like array->data[x][y]? \$\endgroup\$
    – Gilfoyle
    Commented Dec 14, 2022 at 8:07
  • 1
    \$\begingroup\$ @Gilfoyle I have included code snippet to show how to allocate the data on stack as well, but you are right it may not be suitable for large data. In ArrayPrint you really dont need access with x,y pair, you could use ArrayGet(array,x,y) but it is inefficient in the full sequential traversal because it computes i from x and y when i is already known. You should need multidimensional accessors mostly for random access. \$\endgroup\$
    – slepic
    Commented Dec 14, 2022 at 9:42
  • 3
    \$\begingroup\$ OP's goal of two-dimensional array certainly implies access via x->data[r][c] or the like. This answer's alternative does not meet that goal - even if this approach is considered better. Should the next steps in OP coding need to swap rows of data, this approach needs a memcpy() of entire rows vs. simple pointers swap. Although many useful insights here, this answer does not meet OP's stated goal. \$\endgroup\$
    – chux - Reinstate Monica
    Commented Dec 14, 2022 at 15:10
  • \$\begingroup\$ @chux-ReinstateMonica All what matters is whether OP improves his knowledge. I, as reviewer, am not obliged to address each and every aspect of OP's question. Nor am I disallowed to address things OP didn't ask about. Should the next steps of OP be to implement a 3D array he might need something else also. OP made no signs as to what the next steps would be and so there is no need to assume next step is switching rows \$\endgroup\$
    – slepic
    Commented Dec 16, 2022 at 19:03
5
\$\begingroup\$

Small review point:

Although unusual, consider allowing a rows and/or cols of 0. It is not that hard to support and allows greater functionality.

When a dimension is 0, consider not calling malloc(0) and simply assign NULL where needed.

struct Array *NewArray(const size_t rows, const size_t cols){
  //if (rows <= 0 || cols <= 0){
  //    return NULL;
  //}

  // Other additional adjustments.
\$\endgroup\$
4
\$\begingroup\$

Much discussion is about minimizing the number of allocations.

If the 2D organization of the struct Array is meant to be fixed during the lifetime of the object, only 1 allocation is needed. See below. Changing OP's struct Array is not needed. Rows of data can still be swapped, within the struct Array. Code can continue to conveniently access elements via array->data[r][c] on the left and right of an =.

To accomplish 1 allocation, that allocation needs to account for the space of struct Array, an array of double * and an array of double as well as potential padding.

// Allocate once for the following
Space for `struct Array`
Potential padding
double row[rows];
Potential padding
double vals[rows * cols];

Be warned that it is easy to mess up the calculations.

// Staying with OP's original definition and function interfaces (mostly).

struct Array {
  double **data;
  size_t rows;
  size_t cols;
};

static size_t size_round_up(size_t sz, size_t alignment) {
  return (sz / alignment + !!sz % alignment) * alignment;
}

static void* ptr_offset(void *ptr, size_t offset) {
  return (char*) ptr + offset;
}

struct Array* NewArray(const size_t rows, const size_t cols) {
  size_t array_size = sizeof(struct Array);
  size_t rows_offset = size_round_up(array_size, _Alignof(double*));
  size_t rows_size = sizeof(double*) * rows;
  size_t vals_offset = size_round_up(rows_offset + rows_size, _Alignof(double));
  size_t vals_size = sizeof(double) * rows * cols;
  struct Array *all = calloc(1, vals_offset + vals_size);
  if (all) {
    all->data = ptr_offset(all, rows_offset);
    all->rows = rows;
    all->cols = cols;
    double *vals = ptr_offset(all, vals_offset);
    for (size_t r = 0; r < rows; r++) {
      all->data[r] = vals;
      vals += cols;
    }
  }
  return all;
}


void FreeArray(struct Array *array) {
  // With one allocation, freeing is simple.
  // Tolerate FreeArray(NULL).  Just like free(NULL) is OK
  free(array);
}
\$\endgroup\$
11
  • 2
    \$\begingroup\$ You might be able to simplify somewhat by making the first variable-length array a flexible array member (which the compiler will align for us), so we only need to hand-align the second one. \$\endgroup\$
    – Toby Speight
    Commented Dec 15, 2022 at 7:51
  • 1
    \$\begingroup\$ Oh, and calloc() isn't guaranteed to produce valid floating-point values, unless we're on and IEEE-758 (IEC-559) system. \$\endgroup\$
    – Toby Speight
    Commented Dec 15, 2022 at 8:11
  • 3
    \$\begingroup\$ Simpler version, using flexible array member for data: godbolt.org/z/eoKcb84n8 \$\endgroup\$
    – Toby Speight
    Commented Dec 15, 2022 at 8:29
  • 2
    \$\begingroup\$ @TobySpeight True about simplification yet i wanted to avoid changing OP's struct Array and also present an approach that is clearly extensible to 4, 5, 6 ... N parts. \$\endgroup\$
    – chux - Reinstate Monica
    Commented Dec 15, 2022 at 9:23
  • 2
    \$\begingroup\$ @TobySpeight Agree about zero bit pattern is not specified as 0.0 - yet 1) it is very pervasive (like ASCII) and 2) no mention of 0.0 requirement was offered - just wanted a stable initial state. ;-) \$\endgroup\$
    – chux - Reinstate Monica
    Commented Dec 15, 2022 at 9:26
4
\$\begingroup\$

Allocate the Array in one go

The technique below uses a flexible array member at the end of the Array to represent all the elements.

As a consequence of allocating in one go, you can deallocate with a single free.

struct Array {
    size_t rows;
    size_t cols;
    double data[];
};

struct Array * NewArray (size_t rows, size_t cols) {
    size_t sz = sizeof(struct Array) + rows * cols * sizeof(double);
    struct Array *a = malloc(sz);
    if (a) {
        a->rows = rows;
        a->cols = cols;
    }
    return a;
}

void FreeArray (struct Array *a) {
    free(a);
}

Use VLA type to simplify data access

The technique below uses a macro to present an indexable 2D array. The technique casts data into a VLA. The upcoming C.23 standard will make support for variably modified types mandatory. This makes it okay to define a "pointer to a VLA". VLA objects with automatic storage remain optional in C.23.

#define ArrayDataGeneric(a)                                          \
        _Generic((a),                                                \
        struct Array *: (double (*)[(a)->rows][(a)->cols])(a)->data, \
               default: 0)

#define ArrayData(a) (*(ArrayDataGeneric(a)))

This simplifies access to your array by removing explicit access to the data member. You can treat the ArrayData macro result like a regular array.

  ArrayData(a)[1][1] = 1.1;

The last line of a function returning void does not need a return statement.

void PrintArray(const struct Array *a) {
    for (int i=0; i < a->rows; ++i) {
        for (int j=0; j < a->cols; ++j) {
            printf("%.2lf ", ArrayData(a)[i][j]);
        }
        printf("\n");
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Can you say someting about how efficient this approach is? I wonder if the macro comes with more overhead compared to plain array[i][j]? \$\endgroup\$
    – Gilfoyle
    Commented Dec 17, 2022 at 19:58
  • 1
    \$\begingroup\$ @Gilfoyle I didn't think you were concerned that much about performance, since you were following two pointer indirections for resolve a value in your own implementation. A macro is inlined by the compiler. The inlined code is more a compile time check than introducing instructions. The point instructions are created is when the array members are accessed, but that has to be done anyway to find out the number of columns. \$\endgroup\$
    – jxh
    Commented Dec 17, 2022 at 20:07
  • 1
    \$\begingroup\$ If your arrays are small and you understand the locality of the allocations, it may not matter much how the arrays are implemented in terms of array access time. Only a thorough profiling exercise that accurately simulates real world conditions could give you a proper answer on performance. I see that my macro implementation only requires two indirections vs. three to access the double pointer. \$\endgroup\$
    – jxh
    Commented Dec 17, 2022 at 20:45
  • \$\begingroup\$ tio.run/##rZLLasMwEEX3@ooL2cgmCvE2SQOFQpdddNOSmKDISiIwNkhy6/… \$\endgroup\$
    – jxh
    Commented Dec 17, 2022 at 20:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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