gbadev.org forum archive

This is a read-only mirror of the content originally found on forum.gbadev.org (now offline), salvaged from Wayback machine copies. A new forum can be found here.

C/C++ > Best practice for Macros?

#168820 - Drovor - Wed May 27, 2009 9:40 pm

I am working on a simulation project on the DS and essentially have a 2D array of tiles, each tile is a struct of values. The generic tile has a TYPE field and can be various TYPEs (like barrier, empty, food, egg, ... all of which are declared in an enum). Actions can be performed on the struct which modify its properties, like if the TYPE == food, the food can be picked up and the TYPE is changed to empty. (This design may be questionable, but it is working for me surprisingly well and the question is about use of macro's not design!)

These are the relevant values of my tile:
Code:
struct Tile
{
  short TYPE;
  Creature* occupant_one;
  Creature* occupant_two;
};


At first I was using simple comparison macro's to test the TYPEs:
Code:
#define FOOD(X) FOODi(X->TYPE) // Tile* X
#define FOODi(X) ((X == FOOD))


This was very convenient because I was able to expand it without touching any other part of the program when I wanted to make more types of food:
Code:
#define FOOD(X) FOODi(X->TYPE)
#define FOODi(X) ((X == FOOD) || (X == FOOD2))


Well this practice seems to be a slippery slope, because I'm adding more and more of these. All are simply manipulating the struct, but they are getting more complicated than simple comparisons.

For example, I needed a reference to who is standing on the tile in the tile (there are thousands of characters, so a lookup from my list of characters isn't feasable if I need to know something like who is standing next to someone else). Because of that I needed to keep the struct updated when a character moves from one tile to the next and I get something like this:

Code:
#define AVAILABLE_SPOT(X) ((X->occupant_one == '\0') || (X->occupant_two == '\0'))
#define SET_SPOT(X, Y) {if(AVAILABLE_SPOT_ONE(X)) SET_SPOT_ONE(X, Y); else if (AVAILABLE_SPOT_TWO(X)) SET_SPOT_TWO(X, Y);}

#define REMOVE_SPOT(X, Y) {if(SPOT_ONE_IS(X, Y)) SET_SPOT_ONE(X, '\0'); else if (SPOT_TWO_IS(X, Y)) SET_SPOT_TWO(X, '\0');}

#define AVAILABLE_SPOT_ONE(X) (X->occupant_one == '\0')
#define AVAILABLE_SPOT_TWO(X) (X->occupant_two == '\0')

#define SET_SPOT_ONE(X, Y) (X->occupant_one = Y)
#define SET_SPOT_TWO(X, Y) (X->occupant_two = Y)

#define SPOT_ONE_IS(X, Y) (X->occupant_one == Y)
#define SPOT_TWO_IS(X, Y) (X->occupant_two == Y)


Again I can easily update the macro if I wanted to make it so 3 characters can occupy one tile, but then it gets even bigger.

Finally the question:
For those of you who may be more familiar with with using macros, I am wondering what your opinions on them are? I suppose at some point you should just make functions to do this sort of manipulation and I probably crossed that line long ago, where do you consider that line?

#168821 - Dwedit - Wed May 27, 2009 9:48 pm

Functions like this might be a better idea than macros:

static __inline bool isFood(int X)
{
return X == FOOD;
}

Because if you end up going to a List of possible things to match, you can use more code without the restrictions of a macro.
_________________
"We are merely sprites that dance at the beck and call of our button pressing overlord."

#168823 - kusma - Wed May 27, 2009 10:03 pm

Drovor wrote:
For those of you who may be more familiar with with using macros, I am wondering what your opinions on them are? I suppose at some point you should just make functions to do this sort of manipulation and I probably crossed that line long ago, where do you consider that line?

I draw the line at "Do I absolutely HAVE TO use macros to get this to work?". Macros icky to debug, and provide no type-safety.

#168827 - Pete_Lockwood - Thu May 28, 2009 1:10 am

As much as you're saying the question is about macros, I'd suggest that a more complete answer would necessarily address the data structures you're using.

For example for your requirement to know which characters are located on a given tile, you're using two different variables - occupant_one and occupant_two. If you had an array of Creature* in your Tile and a count of used Creature* locations, you'd be able to write cleaner code like this:

Code:

#define MAX_CREATURES 2
#define AVAILABLE_SPOT(X) (X->used_locations < MAX_CREATURES)
#define SET_SPOT(X, Y)    (if(X->used_locations < MAX_CREATURES) X->creature[x->used_locations++] = Y)


The REMOVE_SPOT would be a little longer - you'd trot along the array looking for Y, remove it, trot along the rest of the array moving each character down one, and finally decrement used_locations - but this approach could expand to having 50 creatures in the same spot without any change to the code.

Similarly the question of 'food'. Will your program really have so much data that it's necessary to compress all the data into one variable at the cost of complicating the code? If you have several types of food you could use a boolean in the struct that is only set for 'food' tiles. Then have another variable in the struct which holds an index to the specific 'flavor' of food. That index could also be used indicate the type of terrain for a non-food tile, although I'd guess that perhaps you want your tile to assume a type of terrain after the food has been collected? If that was the case, why not just add another variable to the struct and keep the idea of 'food' separate from terrain?

There are a hundred ways to skin a rabbit and these aren't necessarily the best, the idea is more to get you to consider that your problem might not be whether to write macros but whether it's worth thinking the data structures through more - sooner rather than later.

Personally, if a macro does much more than a little bit manipulation I'd just write an inline function. I'm not sure I can think of any really good reasons to use macros - there are several gotchas you have to avoid when using macros: the lack of type safety, argument re-evaluation, and lack of debugging being the obvious ones - so why bother?
_________________
It's not an illusion, it just looks like one.

#168831 - elwing - Thu May 28, 2009 9:00 am

Pete_Lockwood wrote:
Personally, if a macro does much more than a little bit manipulation I'd just write an inline function. I'm not sure I can think of any really good reasons to use macros - there are several gotchas you have to avoid when using macros: the lack of type safety, argument re-evaluation, and lack of debugging being the obvious ones - so why bother?

you are generally right, trough macro can be used to add readability to the code at the expense of code clarity... and in some place you can not just use inline function (think protothread and such awfull things...). here is an example i've personnally seen (but feel free to check protothread, it's not that different...)

I had to write an almost sequential state machine that was manageing a remote device configuration. I had a sequential list of things to perform (eg. read x, write y...) and each transaction needed to use a substate machine. the main state machine was written as a method containing a big switch case with every case being a state and all of its state looked the same... call the substate machine, depending on the return either switch to the next state (the transaction was ended) or the transaction is pending, return and continue from the same state...
so basically each "state" was looking like:
Code:

      switch(state){
      case STATE_A:
         switch(lFSM(WRITE,&data)){
         case PENDING:
            break;
         case RESULT_OK:      
            state = STATE_B;
            break;
         case ERROR:
            ...
         default:
            ...
         }
      case STATE_B:
         switch(lFSM(WRITE,&data)){
         case PENDING:
            break;
         case RESULT_OK:      
            state = STATE_C;
            break;
         case ERROR:
            ...
         default:
            ...
         }
      ...



if the transmition FSM return that the transmission is pending we get out and it will be called once more using re-entrency (the main FSM is evaluated periodically...)
now, using macro you can tradeoff all theses lines with something clearer to the reader (but much harder to debug... that's a tradeoff...) using macro like:
Code:

#define WRITEFSM(data,nextstate)                  \
      witch(ltrsmFSM(WRITE,&data)){               \
   case PENDING:                           \
      break;                           \
   case RESULT_OK:                        \
      state = nextstate;                     \
      break;                           \
   case ERROR:                           \
      ...                              \
   default:                              \
      ...                              \
   }

that can reduce your FSM code to something like:
Code:

      switch(state){
      case STATE_A:
         WRITEFSM(data,STATE_B);
      case STATE_B:
         WRITEFSM(data,STATE_C);
      ...


it's obviously harder to debug, but for a reader point of view, there's no comparision i think...

now in my opinion, if you except theses special cases, you should only use macro for simple bitwise operation and inline method for all other repetitive threatment, but, in any cases, please avoid using a macro to access a member, in my opinion basket->fruit is much better than FRUIT_IN(basket)...


edit: protothread link http://www.sics.se/~adam/pt/

#168832 - gauauu - Thu May 28, 2009 2:49 pm

For the most part, I think elwing is on a good track. I had similar issues where there were repeating chunks of stuff (especially in some data definitions) that couldn't reasonably be broken into functions, where macros made it a lot more semantic for the human writer/reader of the code.

Another place I ended up using macros was to give more semantic meaning to reusable variables -- I had a system for enemies (using C, not C++) where there was an enemy struct. Something like (albeit rather simplified):
Code:

struct Enemy{
   Character * characterInfo;
   int stat1;
   int stat2;
   int stat3;
   int stat4;

   int (*enemyTypeScript)(int param, Enemy * enemy);

} ;


In this case, all enemies used the same struct, and particular enemy scripting/AI was handled in the enemyTypeScript, with a different function for each enemy type. The stat variables were generic variables that could be used differently for different enemy types. (For some enemies, stat1 was a delay timer, for others, it was a counter for how many bullets they shot). In this case, in the enemyTypeScripts, I used a macro to make it easier to refer to stat1 by "DELAYTIMER", so it would be clearer.

Quote:

you should only use macro for simple bitwise operation and inline method for all other repetitive threatment


Although I do it this way also, what makes bitwise operations special that you don't use inline functions for them?

#168834 - elwing - Thu May 28, 2009 3:14 pm

gauauu wrote:
Quote:

you should only use macro for simple bitwise operation and inline method for all other repetitive threatment


Although I do it this way also, what makes bitwise operations special that you don't use inline functions for them?

not sure, that's actually a good question... it's maybe because you can use the same macro for any integer type...

#168836 - Pete_Lockwood - Thu May 28, 2009 3:57 pm

@Elwing: I'm not following why you'd elect to move the FSM subcode into Macros instead of functions. Doesn't moving it into functions produce the same result? The main FSM code would be highly readable but you wouldn't lose type safety, debuggability(TM) etc.

@gauauu: Without seeing more of the code to know why you didn't, an outside observer might suggest that you could achieve the readability by simply using unions of structs.


The reason I don't mind putting bit operations into a macro is that I'm effectively stating that my macro is probably doing something a little cunning that isn't going to be obvious if it's expanded in code anyway - but I never want the compiler to override my decision and not inline the code.
_________________
It's not an illusion, it just looks like one.

#168838 - Miked0801 - Thu May 28, 2009 6:10 pm

There are ways to make sure that code always inlines. Tha said, if you are being so cunning, one hopes you don't fool yourself later on when debugging or extending your code :)

Seriously, the only time I use macros anymore are for their identifier stitching stuff when build tables:

Code:

#define MODEL_INFO(model) \
   (ModelInfo){ FooCast(model##_foo1), FooCast(model##_foo2), FooCast(model##_foo3) }


Then I just need to have MODEL_INFO(bleh) in a table instead of all the sub components.

#168845 - elwing - Fri May 29, 2009 7:42 am

Pete_Lockwood wrote:
@Elwing: I'm not following why you'd elect to move the FSM subcode into Macros instead of functions. Doesn't moving it into functions produce the same result? The main FSM code would be highly readable but you wouldn't lose type safety, debuggability(TM) etc.


guess I wasn't clear enough, the FSM is in a function and the substate machine is also in a function, but depending on the return I need either to change the main FSM state (no change when pending, move to a certain state in case of error and move to a last state if the transmission is ended) and exit the main FSM. now I could change the main FSM state in the sub FSM function and return in anycase, but changing the main FSM state outside of the FSM code itself seems quite wrong to me... (and not easier to debug).

now if i missed an interesting alternative it would be great if you could point it.
Using macro is not right or wrong, it all depend of what are the gain (readability) and what are the loss (debugability(c) :))

#168846 - sgeos - Fri May 29, 2009 8:43 am

Use macros instead of magic numbers. =)
Macros are also good for stringization and tokenization.
I also use macros when I need to do something many times and I can not reasonablely break the code off into a subroutine due to scope issues.

This is probably not the best example, but here is an example:
Code:
#include <stdio.h>

#define COUNT_ROTTING(a) ((a) < 0 ? -(a) : 0)
#define COUNT_FRESH(a)   (0 < (a) ?  (a) : 0)
#define NAME_ROTTING    "rotting"
#define NAME_FRESH      "fresh"
#define PRINT_FRUIT_NONE(basket,fruit) \
        if (0 == basket->fruit) \
                printf("%d x %s\n", 0, #fruit)
#define PRINT_FRUIT_OF_TYPE(basket,fruit,status) \
        if (0 < COUNT_##status(basket->fruit)) \
                printf("%d x %s %s\n", COUNT_##status(basket->fruit), \
                NAME_##status, #fruit)
#define PRINT_FRUIT(basket,fruit) \
        PRINT_FRUIT_NONE(basket,fruit); \
        PRINT_FRUIT_OF_TYPE(basket,fruit,FRESH); \
        PRINT_FRUIT_OF_TYPE(basket,fruit,ROTTING)

struct fruitBasket
{
        int apple;
        int banana;
        int peach;
        int orange;
        // a bunch of other fruits
};

void printFruitBasket(struct fruitBasket *pBasket)
{
        PRINT_FRUIT(pBasket, apple);
        PRINT_FRUIT(pBasket, banana);
        PRINT_FRUIT(pBasket, peach);
        PRINT_FRUIT(pBasket, orange);
}

int main(void)
{
        struct fruitBasket myFruitBasket = {-1, 0, 1, 2};
        printFruitBasket(&myFruitBasket);
        return 0;
}

Any time you start naming variables with numbers in them (flag1, flag2, actor_one, actor_two), it is a good sign you should probably be using an array.

Also, the macro VS subroutine argument strikes me as moot for the most part. It seems like your macros should really be methods.
Code:
if (myTile.contains(FOOD))
  // pick up food

#168851 - gauauu - Fri May 29, 2009 6:35 pm

Pete_Lockwood wrote:

@gauauu: Without seeing more of the code to know why you didn't, an outside observer might suggest that you could achieve the readability by simply using unions of structs.


Yeah, that would also work. I found my approach to be easier for me to read when I considered both options (partially due to the fact that at the declaration of "enemy" I don't know how many different kinds of specific enemies there will be), but it would definitely be a reasonable choice.

sgeos wrote:
Any time you start naming variables with numbers in them (flag1, flag2, actor_one, actor_two), it is a good sign you should probably be using an array.


True. Really, in my case, it should have been an array of u32's, to be divided up in any way the specific enemy type requires. Practically, in this limited case, it didn't make much difference.