tags:

views:

1660

answers:

10

For example, never define a macro like this:

#define DANGER 60 + 2

This can potentially be dangerous when we do an operation like this:

int wrong_value = DANGER * 2; // Expecting 124

Instead, define like this because you don't know how the user of the macro may use it:

#define HARMLESS (60 + 2)

The example is trivial, but that pretty much explains my question. Are there any set of guidelines or best practices that you would recommend when writing a macro?

Thanks for your time!

+4  A: 

In the expansion, put parenthesis around the arguments, so that if they pass in a expression you will get the intended behavior.

#define LESS_THAN(X,Y) (((X) < (Y) ? (X) : (Y))
EvilTeach
thisw has the ++/-- bug
dsm
Ya. Don't do that.
EvilTeach
+19  A: 

When doing a macro that is to run its argument and behave like an expression, this is idiomatic:

 #define DOIT(x) do { x } while(0)

This form has the following advantages:

  1. It needs a terminating semicolon
  2. It works with nesting and braces, e.g. with if/else
unwind
+5  A: 

use static const values instead of macros for constant values, integral or other. The compiler can often optimize them away, and they remain a 1-st class citizen in the language's type system.

static const int DANGER = 60 + 2;
John Dibling
Will that really work in standard C? In a header file?
Roddy
@Roddy: no, it won't work in C in a header - use an enum instead.
Jonathan Leffler
@John: it depends on the context as to whether this is appropriate.
Steve Melnikoff
+14  A: 

Not only should you put parens around the arguments, you should put parens around the expression returned.

#define MIN(a,b)  a < b ? a : b     // WRONG  

int i = MIN(1,2); // works
int i = MIN(1,1+1); // breaks

#define MIN(a,b)  (a) < (b) ? (a) : (b)   // STILL WRONG

int i = MIN(1,2); // works
int i = MIN(1,1+1); // now works
int i = MIN(1,2) + 1; // breaks

#define MIN(a,b)  ((a) < (b) ? (a) : (b))   // GOOD

int i = MIN(1,2); // works
int i = MIN(1,1+1); // now works
int i = MIN(1,2) + 1; // works

However, MIN(3,i++) is still broken...

The best rule is only to use #defines only when NO OTHER APPROACH WILL WORK! I know you're asking about C instead of C++, but still bear his in mind.

Roddy
Maybe you should add a note about using enum instead of defining numeric constants with #define.
Jonathan Leffler
Ack! MIN(3, i++)! I never considered that case... That's gotta suck to debug that!
Adam Davis
Of course, the fix is trivial - make it a function, or take the increment out of that line of code. I don't agree that macros shouldn't be used except when no other approach will work - Like any other tool macros can be dangerous if improperly used, but done well they can decrease development time
Adam Davis
+2  A: 

Use fairly unique names for your macros, since they have global scope and can clash with anything, so:

#define MAX 10

could easily clash with other code, so:

#define MYPROJECT_MAX 10

or something even more unique, would be better.

I've seen cases where a clash of this kind didn't produce a compile error, but generated slightly wrong code, so it can be quite insidious.

DarthPingu
+5  A: 

Use parenthesis around the entire macro and around each argument referred to in the expansion list:

#define MAX(x, y) ((x) > (y) ? (x) : (y))


Avoid writing macros that evaluate their arguments multiple times. Such macros will not behave as expected when arguments have side effects:

MAX(a++, b);

Will evaluate a++ twice if a is greater than b.


Use UPPERCASE names for macros to make it clear it is a macro and not a function so that the differences can be considered accordingly (another general good practice is not passing arguments that have side effects to functions either).


Don't use macros to rename types like this:

#define pint int *

because it won't behave as expected when someone types

pint a, b;

Use typedefs instead.

Robert Gamble
+2  A: 

If you're careful and expert, you may be able to accomplish DRY (Don't-Repeat-Yourself) code, by using macros as simple code generators. You do have to explain to other programmers what you're doing, but it can save a lot of code. For example, the list-macro technique:

// define a list of variables, error messages, opcodes
// or anything that you have to write multiple things about
#define VARLIST \
    DEFVAR(int, A, 1) \
    DEFVAR(double, B, 2) \
    DEFVAR(int, C, 3) \

// declare the variables
#define DEFVAR(typ, name, val) typ name = (val);
    VARLIST
#undef  DEFVAR

// write a routine to set a variable by name
void SetVar(string varname, double value){
    if (0);
    #define DEFVAR(typ, name, val) else if (varname == #name) name = value;
        VARLIST
    #undef  DEFVAR
    else printf("unrecognized variable %s\n", varname);
}

// write a routine to get a variable's value, given its name
// .. you do it ..

Now, if you want to add a new variable, delete one, or rename one, it's a 1-line edit.

Mike Dunlavey
I hope I never have to maintain any code that you write using this idiom.
Jonathan Leffler
Like I said, it takes explanation, which is true of anything but lowest-common-denominator code. If you want to do code generation, it is a way.
Mike Dunlavey
there is a places where it's acceptable. But one should be very careful using this technique.
Ilya
Right. Not every useful idea is popular or well-known.
Mike Dunlavey
+2  A: 

Response to the MAX/MIN macros, taken from GCC hacks in the Linux kernel:

#define min(x, y) ({                       \
        typeof(x) _min1 = (x);             \
        typeof(y) _min2 = (y);             \
        (void) (&_min1 == &_min2);         \
        _min1 < _min2 ? _min1 : _min2; })
dalle
typeof is a gcc extension and should not be used in portable code.
Robert Gamble
Agreed with Robert. Does anyone want to explain what the void cast and expression is there for?
Jonathan Leffler
Also, the use of ({ ...; foo;}) as evaluating to foo. That's a new one on me.
Mike Dunlavey
I wonder if the void cast and expression is trying to tell the compiler that _min1 and _min2 have to be compatible scalars and not be in registers?
Mike Dunlavey
"the use of ({ ...; foo;})" is another GCC extension. That's why it's a GCC hack, not a C hack. Not sure what else, but the void thing stops you using one signed and one unsigned argument. The types of x and y would be compatible, but their pointer types aren't.
Steve Jessop
@onebyone: thank you for the explanation of the void cast.
Jonathan Leffler
Boy, that's subtle.
Mike Dunlavey
A: 

See how i hate this:

void bar(void) {
    if(some_cond) {
        #define BAZ ...
        /* some code */
        #undef BAZ
    }
}

Always put them like this:

void bar(void) {
    if(some_cond) {
#define BAZ ...
        /* some code */
#undef BAZ
    }
}
Johannes Schaub - litb
While I agree with you, this has nothing to do with macros definitions.
Robert Gamble
right. i've corrected that. thanks :)
Johannes Schaub - litb
I wish my gripes were as big as that :-)
Mike Dunlavey
well i don't like it :) same with goto labels
Johannes Schaub - litb
A: 

Undefine your marcos.

Your #defines should matched with an #undef. This prevent the preprocessed from getting clogged up and affecting unintended pieces of code.

lillq