current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

I try to work with a contiguous block of memory, to create an array (2D) whose SIZE is not known at compile time (before ) so no variable-length arrays are involved here.

I came up with the following:

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

int main(void){
    unsigned int row, col,i, j, k;
    int l = 0;
    int *arr;

    printf("Give the ROW: ");
    if ( scanf("%u",&row) != 1){
        printf("Error, scanf ROW\n");
        exit(1);
    }

    printf("Give the COL: ");
    if ( scanf("%u",&col) != 1){
        printf("Error, scanf COL\n");
        exit(2);
    }

    arr = malloc(sizeof *arr * row * col);
    if(arr == NULL){
        printf("Error, malloc\n");
        exit(3);
    }

    for ( i = 0; i < row ; i++){
        for ( j = 0 ; j < col ; j++){
            arr[i * col + j] = l;
            l++;
        }
    }

    for (k = 0 ; k < (row * col) ; k++){
        printf("%d ",arr[k]);
    }

    free(arr);
}

Which gives me the following:

Give the ROW: 6
Give the COL: 3
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17

Is this the right approach?

share|improve this question
    
Yes, this is the right approach. This is known as "mangled arrays" and was the only way you could allocate 2D arrays dynamically before C99, if you didn't know the size at compile time. This was a major flaw in C90, because it means you have to calculate the dimensions manually in run-time, every time you wish to access "array[x][y]". One of many reasons to use a modern compiler instead. VLA:s might be mildly useful, but pointers to VLA:s are very useful. They would have made this code so much more readable (and possibly also slightly faster). – Lundin May 25 at 7:51
    
Also regarding 2D arrays, make sure to note that the order of iteration of nested for loops matters a lot, when it comes to performance on computers with data cache. Not an issue in this code, but it is good to know. See this. When allocating memory dynamically, cache performance is one of the main reasons why you should pick mangled arrays over "emulated 2D arrays" through pointer-to-pointer look-up tables. – Lundin May 25 at 7:52
up vote 3 down vote accepted

Of course this in not a true 2d array, just a 1d array indexed using arr[i * col + j].

This is an OK approach. When able, I prefer an array of pointers to an array of ints, as it is usually more flexible to the tasks I tackle. YMMV.

  1. Modest extreme case: sizeof *arr * row * col is multiplied in the right order. When row or col is int/unsigned, good to use sizeof *arr first as that is type size_t and the subsequent multiplication will then happen at least using size_t math. The concern being that with unsigned row, col, row*col calculated first could overflow using unsigned math, but not size_t math.

    Pedantically, even sizeof *arr * row * col can overflow. Should code need to detect this, the reasonable approach is to use the widest unsigned types like unsigned long long or uintmax_t. (Other more pedantic methods exist.) 

    assert(1LLU * sizeof *arr * row * col < SIZE_MAX);

  2. Suggest using size_t for row, col, i, j instead of any other type. size_t is the Goldilocks type for array indexes - not too narrow, not too wide.

  3. Error messages are better sent to stderr. 2 advantages: will not get lost in code's typical stdout output and the stderr stream is flushed ensuring output is not mixed after later input.

  4. Idea: Using a arr = 0; after the free(arr); has merit when the free'd variable is still in scope for a long time. Although not needed, a decent compiler will optimized it out, but errant subsequent use is easier to detect when arr == NULL than arr = some_freed_value.

  5. Minor: printf("%d ",arr[k]); in a loop is sometimes a problem when trying to print more than some environmental limit number (e.g. 4095) of characters on the same line. Liberal use of fflush(stdout); or including a '\n' in the text usually solves this.

  6. A key comment missing is: is this array row or column major? With some analysis, that is discernible, but from a header only interface, knowing which dimension moves in the smallest steps is important in how higher level code will want to use this array for cache hit efficiency.

  7. Coding style: No need to introduce a variable until ready for assignment. Consider the 2nd style.

    // int *arr;
// ... many lines
// arr = malloc(sizeof *arr * row * col);

... many lines
int *arr = malloc(sizeof *arr * row * col);
share|improve this answer
    
Also please note that when changing the type to size_t, you also have to change the printf conversion specifiers to %zu. – Lundin May 25 at 7:47
    
unsigned long long is size_t on my platform. An overflow check for a * b * c (all unsigned) looks like this: a <= SIZE_MAX / b / c. Overall a good review. +1 – David Foerster May 25 at 12:07
    
@David Foerster Thanks for the simple test idea. Given corner cases and integer division truncates, a < SIZE_MAX / b / c (< vs <=) might better catch overflow edge values. IAC, a*b*c near SIZE_MAX is a bit tricky. – chux May 25 at 13:57

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.