Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Looks like you need a function void swap(int*, int*)! See also Implement generic swap macro in CImplement generic swap macro in C. Writing three lines of code by hand every time you need to swap two values will bite you eventually, if you do it long enough.

Looks like you need a function void swap(int*, int*)! See also Implement generic swap macro in C. Writing three lines of code by hand every time you need to swap two values will bite you eventually, if you do it long enough.

Looks like you need a function void swap(int*, int*)! See also Implement generic swap macro in C. Writing three lines of code by hand every time you need to swap two values will bite you eventually, if you do it long enough.

Source Link
Quuxplusone
  • 19.4k
  • 2
  • 42
  • 88

Tangential request: Please pick useful (long if necessary) names for your source files. A very good start would be to take the "decoder ring" you posted to comp.lang.c and make the appropriate substitutions throughout.

ar ==> array
en ==> encoding
st ==> symbol_table
wd ==> scanner (or perhaps "lexer", if that's what you mean)
vb ==> verbs (or perhaps "operators", or perhaps "verbs_and_operators")

You're using __VA_ARGS__, so this must be C99 (or better). You should take advantage of C99's "declaration at point of use" syntax: i.e., instead of the K&R-C-looking

int productdims(int rank, int dims[]){
    int i,z=1;
    for (i=0; i<rank; i++)
        z *= dims[i];
    return z;
}

you should write

int product_of_dims(int rank, const int *dims)
{
    int z = 1;
    for (int i=0; i < rank; ++i) {
        z *= dims[i];
    }
    return z;
}

Notice that in addition to declaring i within the for-loop, I also

  • const-qualified things that can safely be const
  • added whitespace, for readability
  • added curly braces around even one-line control structures, for consistency
  • replaced [] with * in parameter declarations; [] is a misleading syntax, since what's being passed is not an array but merely a pointer.

I'll rewrite your next function, too, to show what it could be like:

array *new_array_with_dims(int rank, const int *dims)
{
    const int datasz = product_of_dims(rank, dims);
    struct array *z = malloc(sizeof *z + (2*rank+datasz)*sizeof(int));
    z->type = normal;
    z->rank = rank;
    z->cons = 0;

    z->dims = (int *)(z + 1);
    memmove(z->dims, dims, rank * sizeof(int));

    z->weight = z->dims + rank;
    for (int i = 0, wgt = 1; i < rank; ++i) {
        z->weight[rank-i-1] = wgt;
        wgt *= z->dims[rank-i-1];
    }

    z->data = z->weight + rank;

    return z;
}

From 23 lines (21 non-blank) to 21 (17 non-blank), and with a nice self-explanatory name (eliminating one additional comment line), and properly const-qualified.

I split up the initializations so that we conceptually initialize z first, then z->dims, then z->weight, and finally z->data. This isn't so much of a "good style" versus "bad style" thing; it was just a personal organizational choice.


Notice also that I quietly eliminated the confusing typedef struct ar *array in favor of typedef struct array array. Strongly prefer never to create an opaque typedef for a type that isn't a value type. The reader, being human, will instinctively assume that if he has an array a and creates a copy b = a, now he has a copy. If it turns out that what he really has is a handle to a's data (not a new array at all), that's going to be frustrating to him.

Whereas, every C programmer can be expected to know that if you make a copy of array *a with b = a, you get a copy of the pointer. That's much more understandable.

If you are going to use typedefs for struct types, it's good style to write typedef struct X { ... } X; for every struct type in your program. That'll save the maintainer from having to look up for each type X whether he needs to be writing struct X or merely X; the answer is always "merely X". (The C++ language does this for you automatically.)


API design issue: You should avoid creating a pair of names array_new (macro) and array_new (function) that do different things. The Standard Library provides macro/function pairs such as printf and isdigit, but it always makes sure that the macro and the function have exactly the same semantics. You've made it so that array_new(2,2) creates a 2x2 array but (array_new)(2,2) segfaults. This is very bad design.

Call the helper function something like array_new_helper or array_new_impl, so that nobody will ever call it by accident (e.g., through a function pointer, or through a helper macro that wraps its arguments in parentheses for safety).


int t;
t = a->dims[0],
    a->dims[0] = a->dims[1],
                 a->dims[1] = t;

Looks like you need a function void swap(int*, int*)! See also Implement generic swap macro in C. Writing three lines of code by hand every time you need to swap two values will bite you eventually, if you do it long enough.


I'm sure there's more to improve, but I'm out of time. This has probably given you a lot to go on, anyway.