Assignment macro writing

Question

I am trying to write a macro for the following repeated code inside the for loop.

for(i=0; i<n; i++) {
 a->x = b->x;
 a->y = b->y;
 a->z = b->z;
}
for(j=0;j<n;j++){
 a->x = c->x;
 a->y = c->y;
 a->z = c->z;
}

---------
with macro
#define COPY(x,y,z) \
a->x = x;\
a->y = y;\
a->z = z;\

for(i=0;i<n;i++)
 COPY(b->x,b->y,b->z);
for(i=0;i<n;i++)
 COPY(c->x,c->y,c->z);

I keep getting the error unexpected expression before ;

---

thanks for quick responses, Looks like I am more inclined to a function call rather macros, as it might get cumbersome overtime. Are there any performance impacts than using a function call rather than a macro?

Answer 1

I am trying to write a macro for the following repeated code inside the for loop.

Please don't. It is just 3 lines. All you will do is to clutter down your code with obscure and unsafe home-brewed language syntax. You've already managed to write one fatal bug by doing so: not using {} for the macro. Function-like macros are very bad practice in most cases.

First of all, is there a reason why you can't write *a = *b; ?

If there is such a reason, then instead consider doing something like this:

typedef struct // given this struct
{
  int x;
  int y;
  int z;
} xyz_t;

void xyz_copy (xyz_t* dest, const xyz_t* source)
{
  dest->x = source->x;
  dest->y = source->y;
  dest->z = source->z;
}

Answer 2

It is a bad case for macros. You might try

#define COPYxyz(Dst,Src) do {   \
    (Dst)->x = (Src)->x;        \
    (Dst)->y = (Src)->y;        \
    (Dst)->z = (Src)->z;        \
   } while(0)

(notice that the do{...}while(0) is a very old useful trick in macros)

then code COPYxyz(a,b) (but COPYxyz(p++,--q) or COPYxyz(++p,p) is a disaster, this is why coding such a macro is bad)

However, if you have

  struct my_st  {int x, int y, int z};
  struct my_st *a = something();
  struct my_st *b = otherthing();

you could just code a structure assignment:

     *a = *b;

or, assuming you are sure that a and b do not alias (are not the same address) and do not overlap, you could

     memcpy(a, b, sizeof(struct my_st));

and if the structure contains more than x, y, z fields but you only want to copy them, make a proper inline function:

static inline void copy_xyz(struct my_st*dst, const struct my_st*src) {
   assert (src != NULL);
   assert (dst != NULL);
   if (dst == src) return;
   dst->x = src->x;
   dst->y = src->y;
   dst->z = src->z;
   /// remaining fields are not copied!
}

Then you'll code copy_xyz(p++,p) without fears, and it should be as efficient as the COPYxyz macro. You might remove the assert-s and the if (dst == src) return; if you are sure they are useless.

Answer 3

#define COPY(x,y,z) \
a->x = x;\
a->y = y;\
a->z = z;

with this when you pass

COPY(b->x,b->y,b->z);

Then you have

a->b->x = b->x;
a->b->y = b->y;
a->c->y = c->y;;

Which is not what you need. So you need to replace it with

#define COPY(p,q,r) do{\
a->x = p;\
a->y = q;\
a->z = r;\
}while(0)

Answer 4

The macro

#define COPY(x,y,z) \
a->x = x;\
...

will replace x both for both occurences in a->x = x;, e.g. COPY(b->x, ...) gives a->b->x = b->x, so use other names for the macro parameters.

---

Notice that

for(i=0;i<n;i++)
 COPY(b->x,b->y,b->z);

will expand to

for(i=0; i<n; i++)
  a->x = b->x;
a->y = b->y;
a->z = b->z;

which is not what you intend. Make it a habbit to always put macro bodies within do { ... } while (0).

Answer 5

This definition:

#define COPY(x,y,z) \
a->x = x;\
a->y = y;\
a->z = z;\

With a for loop

for(i=0;i<n;i++)
    COPY(b->x,b->y,b->z);
for(i=0;i<n;i++)
    COPY(c->x,c->y,c->z);

Which results in this quick test:

#include <stdio.h>

#define COPY(x,y,z) \
    a->x = x;\
    a->y = y;\
    a->z = z;\

int main( int argc, char* argv[] )
{
    for(i=0;i<n;i++)
        COPY(b->x,b->y,b->z);

    for(i=0;i<n;i++)
        COPY(c->x,c->y,c->z);
}

Only pre-processed with: gcc -E main.c > main.i

Gives

int main( int argc, char* argv[] )
{
    int i;

    for(i=0;i<n;i++)
        a->b->x = b->x; a->b->y = b->y; a->b->z = b->z;;

    for(i=0;i<n;i++)
        a->c->x = c->x; a->c->y = c->y; a->c->z = c->z;;
}

There's a lot wrong!

You have two places where macro replacement's ocurring in a->x = x because both x's get replaced, so firstly change to unique identifiers. Secondly, do not encode the copy-to element in the define. In your case this is a, make this something passed to the macro:

#define COPY( dest, x_copy, y_copy, z_copy ) \
    dest->x = x_copy; \
    dest->y = y_copy; \
    dest->z = z_copy;

You've also fallen into the trap of having if followed by a multiple-statement macro which will obviously not perform how you intended because you've not surrounded the multi-line macro with curly braces! It's common practice to wrap multiple line macros in do{ } while(0) so that they can be used as you would expect:

#define COPY( dest, x_copy, y_copy, z_copy ) \
    do { \
    dest->x = x_copy; \
    dest->y = y_copy; \
    dest->z = z_copy; \
    } while(0)

Now, replacing your macro which this new one results in an output we'd expect to work:

#include <stdio.h>

#define COPY( dest, x_copy, y_copy, z_copy ) \
    do { \
    dest->x = x_copy; \
    dest->y = y_copy; \
    dest->z = z_copy; \
    } while(0)

int main( int argc, char* argv[] )
{
    int i;

    for(i=0;i<n;i++)
        COPY(a, b->x,b->y,b->z);

    for(i=0;i<n;i++)
        COPY(a, c->x,c->y,c->z);
}

Again, gcc -E main.c > main.i results in the correct code expanded from the macro:

int main( int argc, char* argv[] )
{
    int i;

    for(i=0;i<n;i++)
        do { a->x = b->x; a->y = b->y; a->z = b->z; } while(0);

    for(i=0;i<n;i++)
        do { a->x = c->x; a->y = c->y; a->z = c->z; } while(0);
}