Arrays and Functions (how to clean up code)

Hello,

Apparently I have a strange way of writing C. For example, here
is a short program showing how I use arrays in functions:

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

void initialize(int mx, int meqn, double (*p_q)[meqn]);

int main(void) {

int mx = 20,i;
int meqn = 3;
int d1[20];

double q[mx][meqn];
double (*p_q)[meqn];
p_q=q;


initialize(mx, meqn, q);

for (i=0;i<mx;i++){
printf(" i %3d q[i][0] %15.7e\n",i, q[i][0]);
d1[i]=i;
}

return(0);

}

void initialize(int mx, int meqn, double (*p_q)[meqn]){

int i;
double *q;
q = (double *)p_q;


for (i=0;i<mx;i++){
p_q[i][0] = 1.*i;
p_q[i][1] = 2.*i;
p_q[i][2] = 3.*i;
}

}


I was told this was a terrible way to program with unnecessary
variables, etc. I was given the following example of how to clean up
my code and be more efficient in my programming and use dynamic memory
allocation to boot:

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

void initialize(double p_q[][3], int , int );

int main(void) {

double **p_q;


int mx = 20,i;
int meqn = 3;
int d1[20];

p_q = malloc(sizeof(double)*mx*meqn);

initialize(p_q, mx, meqn);

for (i=0;i<mx;i++){
printf(" i %3d p_q[i][0] %15.7e\n",i, p_q[i][0]);
d1[i]=i;
}

return(0);

}

void initialize(double p_q[][3], int mx, int meqn){

int i;

for (i=0;i<mx;i++){
p_q[i][0] = 1.*i;
p_q[i][1] = 2.*i;
p_q[i][2] = 3.*i;
}

}


The problem is that while this second program compiles (with a
warning), it does not run. In fact, it gives a bus error.

So my question is 2-fold: what is wrong with the second program? and
what is the optimal way to write a program like this? Keep in mind
this is a simplified form of a much more sophisticated program
(several thousand lines of code). My thanks in advance.

x

On Mon, 2 Nov 2009 08:46:42 -0800 (PST), x <>
wrote:

>Hello,
>
> Apparently I have a strange way of writing C. For example, here
>is a short program showing how I use arrays in functions:
>
>#include <stdio.h>
>#include <stdlib.h>
>
>void initialize(int mx, int meqn, double (*p_q)[meqn]);

This cannot compile even in C99 since the compiler has no knowledge of
meqn yet.

>
>int main(void) {
>
> int mx = 20,i;

Multiple definitions per line are hard enough to read but without any
spacing even more so.

> int meqn = 3;
> int d1[20];
>
> double q[mx][meqn];
> double (*p_q)[meqn];

Both of these are valid only in C99, not C89.

If you change meqn and mx to #define macros, you eliminate the need
for variable length arrays.

> p_q=q;

You never use p_q.

>
>
> initialize(mx, meqn, q);
>
> for (i=0;i<mx;i++){
> printf(" i %3d q[i][0] %15.7e\n",i, q[i][0]);
> d1[i]=i;
> }
>
> return(0);

return is a statement, not a function. The parentheses are
superfluous.

>
>}
>
>void initialize(int mx, int meqn, double (*p_q)[meqn]){
>
> int i;
> double *q;
> q = (double *)p_q;

You never use q.

>
>
> for (i=0;i<mx;i++){
> p_q[i][0] = 1.*i;
> p_q[i][1] = 2.*i;
> p_q[i][2] = 3.*i;
> }
>
>}
>
>
>I was told this was a terrible way to program with unnecessary

Terrible is a value judgment.

>variables, etc. I was given the following example of how to clean up

You do have unused variables.

By definition, cleaned-up code should at least compile.

>my code and be more efficient in my programming and use dynamic memory
>allocation to boot:
>
>#include <stdio.h>
>#include <stdlib.h>
>
>void initialize(double p_q[][3], int , int );

You need to recognize that the type of p_q here is exactly the same as
the intended type in your original.

>
>int main(void) {
>
> double **p_q;
>
>
> int mx = 20,i;
> int meqn = 3;
> int d1[20];
>
> p_q = malloc(sizeof(double)*mx*meqn);

Since p_q is a double**, it must point to a double*. You allocate
space for a quantity of double but have no idea how the size of a
double relates to the size of a double*.

>
> initialize(p_q, mx, meqn);

This is a constraint violation. p_q is a double** but the first
argument to initialize must have type double(*)[3]. The two types are
incompatible and there is no implicit conversion between them. A
compiler diagnostic is required.

>
> for (i=0;i<mx;i++){
> printf(" i %3d p_q[i][0] %15.7e\n",i, p_q[i][0]);

This is a fatal error. p_q is a double**. Therefore p_q[i] must be a
double* and contain the address of a double. But your initialize
function initializes a 2d array of double and never stores any
addresses, only floating point values.

> d1[i]=i;

What purpose does the d1 array serve?

> }
>
> return(0);
>
>}
>
>void initialize(double p_q[][3], int mx, int meqn){
>
> int i;
>
> for (i=0;i<mx;i++){
> p_q[i][0] = 1.*i;
> p_q[i][1] = 2.*i;
> p_q[i][2] = 3.*i;
> }
>
>}
>
>
>The problem is that while this second program compiles (with a
>warning), it does not run. In fact, it gives a bus error.

The fact that your compiler chooses to call the diagnostic a warning
in no way changes its true severity. You lied to the compiler by
telling it that it could find an address in a location where you never
stored an address. The compiler is naive enough to believe you and
treated the data at that location as if it were an address.
Fortunately, your hardware is slightly less naive and is telling you
that there is no such address from which it can retrieve data.

>
>So my question is 2-fold: what is wrong with the second program? and

Exactly what the compiler told you was wrong. The argument you pass
the function is not compatible with the parameter the function intends
to process.

>what is the optimal way to write a program like this? Keep in mind
>this is a simplified form of a much more sophisticated program
>(several thousand lines of code). My thanks in advance.

You cannot build a sophisticated program unless you understand the
fundamentals of the language. If your initialize function always
deals with the same size arrays, then define the arrays in main and
pass them as you do in your first example. If you want your
initialize function to deal with 2d arrays of different sizes, then
one approach that works is
double* initialize(int rows, int cols){
double *ptr = malloc(rows * cols* sizeof *ptr);
int i, j;
/* test ptr for success */
for (i = 0; i < rows; i++){
for (j = 0; j < cols; j++){
ptr[i*cols+j] = /* your initialization value */;
}
}
return ptr;
}
which would be called in main with something similar to
double *myptr;
myptr = initialize(mx, meqn);

--
Remove del for email

Barry Schwarz

Barry Schwarz <> writes:

> On Mon, 2 Nov 2009 08:46:42 -0800 (PST), x <>
> wrote:
>
>>Hello,
>>
>> Apparently I have a strange way of writing C. For example, here
>>is a short program showing how I use arrays in functions:
>>
>>#include <stdio.h>
>>#include <stdlib.h>
>>
>>void initialize(int mx, int meqn, double (*p_q)[meqn]);
>
> This cannot compile even in C99 since the compiler has no knowledge of
> meqn yet.

Are you sure? It looks fine to me.

<snip>
--
Ben.

Ben Bacarisse

x <> writes:

> Apparently I have a strange way of writing C. For example, here
> is a short program showing how I use arrays in functions:
>
> #include <stdio.h>
> #include <stdlib.h>
>
> void initialize(int mx, int meqn, double (*p_q)[meqn]);
>
> int main(void) {
>
> int mx = 20,i;
> int meqn = 3;
> int d1[20];
>
> double q[mx][meqn];
> double (*p_q)[meqn];
> p_q=q;
>
>
> initialize(mx, meqn, q);
>
> for (i=0;i<mx;i++){
> printf(" i %3d q[i][0] %15.7e\n",i, q[i][0]);
> d1[i]=i;
> }
>
> return(0);
>
> }
>
> void initialize(int mx, int meqn, double (*p_q)[meqn]){
>
> int i;
> double *q;
> q = (double *)p_q;
>
>
> for (i=0;i<mx;i++){
> p_q[i][0] = 1.*i;
> p_q[i][1] = 2.*i;
> p_q[i][2] = 3.*i;
> }
>
> }
>
>
> I was told this was a terrible way to program with unnecessary
> variables, etc.

Well you do have several pointless variables, but there is nothing
wrong with using pointers to arrays. Of course, your code only works
with C99 compilers (unless I've missed what Barry is saying is wrong).

> I was given the following example of how to clean up
> my code and be more efficient in my programming and use dynamic memory
> allocation to boot:
>
> #include <stdio.h>
> #include <stdlib.h>
>
> void initialize(double p_q[][3], int , int );
>
> int main(void) {
>
> double **p_q;
>
>
> int mx = 20,i;
> int meqn = 3;
> int d1[20];
>
> p_q = malloc(sizeof(double)*mx*meqn);
>
> initialize(p_q, mx, meqn);
>
> for (i=0;i<mx;i++){
> printf(" i %3d p_q[i][0] %15.7e\n",i, p_q[i][0]);
> d1[i]=i;
> }
>
> return(0);
>
> }
>
> void initialize(double p_q[][3], int mx, int meqn){
>
> int i;
>
> for (i=0;i<mx;i++){
> p_q[i][0] = 1.*i;
> p_q[i][1] = 2.*i;
> p_q[i][2] = 3.*i;
> }
>
> }
>
>
> The problem is that while this second program compiles (with a
> warning), it does not run. In fact, it gives a bus error.

It's worse. It lies, big time, about the type of p_q. It is declared
as a pointer to a pointer to double, but the data in it is just
doubles. This is almost certain to fail is a bad way.

> So my question is 2-fold: what is wrong with the second program? and
> what is the optimal way to write a program like this? Keep in mind
> this is a simplified form of a much more sophisticated program
> (several thousand lines of code). My thanks in advance.

--
Ben.

Ben Bacarisse