Beware of macros!

Grump

Here's a bug I ran across yesterday:

int iMaxFP = 0;
for (POSITION pos = listFlowPeriods.GetHeadPosition(); pos != NULL; )
    iMaxFP  = max(iMaxFP , listFlowPeriods.GetNext(pos));

Note: listFlowPeriods is an MFC CList<int,int> populated with an arbitrary set of values. The purpose of this code is to determine the largest value in the list. Funnily enough, it actually does work some times! Cheers, Ralph

Electron Shepherd

It's one of the few cases that violates the de facto standard of macros being in upper case. Both min and max look so much like standard CRT functions, it's easy to make that sort of mistake.

Server and Network Monitoring

Nemanja Trifunovic

Exactly. On the other hand, there is no reason for a C++ programmer to use these macros. The standard library provides template functions std::min()[^] and std::max()[^]. The only problem is that the macros can screw up even these functions, but there is a workaround[^] for that. C/C++ preprocessor macros should be banished X|

Programming Blog utf8-cpp

supercat9

C/C++ preprocessor macros should be banished At least in the embedded C world, preprocessor macros can make readable code more practical than it would be otherwise. For example, which is a more legible way of checking whether a timer is expired:

/* Method 1 */ if (TIMER_EXPIRED(my_timer)) do_something();
/* Method 2 */ if (expired_timers & TF_my_timer) do_something;

On something like a PC, I'd probably code something like #1 with a function call, but on the embedded CPU I'm using, using method #1 with a macro, or using method #2 will generate a single-word single-cycle bit-test instruction; using method #1 with a function call would generate a minimum of four words of code (assuming my_timer is a single byte constant--most likely an enum) which would take many cycles to execute (bit-shifting is very slow). Battery life is a major design objective, and the CPU uses far less power when idle than when awake; consequently, letting the unit quickly determine what events need servicing is useful and important. Note, btw, that application code never uses expired_timers or other timer-related variables directly; access is always via the TIMER_* macros. Thus, the application programmer doesn't have to worry about how TIMER_EXPIRED works, or what my_timer represents; on some other CPU, I might very well use a function call, and have my_timer be a simple enumeration type, but that would be transparent to the application programmer.

killabyte

i totally agree with you macros have a definite time and place where they are an excellent choice, cleaning up code to be more readable etc. any programming syntax can be used in a way that is confusing or just plain bad practise, Exhibit A anyone using GOTO/LABEL outside of an ASM project :-\ . one thing i would dispute is that bit shifting is slow... on every embedded platform( and PC) there has been bit shifting instructions which to me says 1 or 2 cycles to execute, which is fast. have i missed something?

zlezj

Thatswhy I always use braces when using if/while/for statements. On the other hand: all multi-line macros could be defined inside a do { ... } while (0) block. This prevents problems like the one mentioned and works correctly with semicolons

Nemanja Trifunovic

supercat9 wrote:

On something like a PC, I'd probably code something like #1 with a function call, but on the embedded CPU I'm using, using method #1 with a macro, or using method #2 will generate a single-word single-cycle bit-test instruction; using method #1 with a function call would generate a minimum of four words of code

I have not used C in a while now, but would expect the compiler to inline the function and produce identical code in both cases. At least, that's what any decent C++ compiler would do.

Programming Blog utf8-cpp

Nemanja Trifunovic

killabyte wrote:

i totally agree with you macros have a definite time and place where they are an excellent choice, cleaning up code to be more readable etc.

I have yet to see such a case, at least with C++.

Programming Blog utf8-cpp

supercat9

killabyte wrote:

one thing i would dispute is that bit shifting is slow... on every embedded platform( and PC) there has been bit shifting instructions which to me says 1 or 2 cycles to execute, which is fast. have i missed something?

On the PIC series microcontrollers, testing a known bit of a byte takes a single cycle. Testing bit 'n' of a byte (where 'n' is variable) is much slower. The best approach for that is probably:

movlw 1
btfsc _bitnumber,1
movlw 4
btfsc _bitnumber,0
rlncf WREG,f,c
btfsc _bitnumber,2
swapf WREG,f,c
andwf _testval,w

but I know of no way to convince a compiler to generate that. More likely would be something like:

; if (testval & (1 << n))
movf _testval,w ; Test if zero
movlw 1
beq done
movwf _temp
shift:
rlcf _WREG,f,c
decfsz _temp
bra shift
done:
andf _testval,w

Testing bit 0 would take 5 cycles. Testing bit 7 would take 33 cycles. And I'm actually being nice by assuming the compiler could tell that 'n' was less than 8 (the code would generate incorrect results for larger 'n'). Generating correct results for larger 'n' would either require adding a cycle to the loop or adding about 3 more instructions. Further, even if the compiler knew how to generate optimal code for the byte case, testing bit 'n' of a 16-bit or 32-bit value would require additional code (it would probably be worth using C code to select a byte to test, since "if (testval & (1 << n))" would be really icky with a 16-bit or 32-bit testval (the loop would take over 200 cycles).

killabyte

ahh yes very good point, when n is a variable it does become a little less simple.

supercat9

I wish that C compilers would support as a common extension a function like __knownconst(x). The function would be evaluated by the compiler, not the preprocessor, and would always evaluate to a constant zero or one; the argument must never be evaluated at runtime. It must return 0 if the value is not known by the compiler to evaluate to a constant, and should (but would not be required to) return 1if the value is not so known. The compiler could also include __knowntrue(x) and __knownfalse(x) which would be equivalent to (__knownconst(!(x)) && (x)) or (__knownconst(!(x)) && !(x)), but with the advantage that code to generate (x) would never be generated (if given the expression (0 && (x)), some compilers will generate code for (x) even though such code will never execute). Such an extension would make it practical to have a macro like:

#define show_something(x) (__knowntrue((x)==0) ? show_zero() : \
(__knowntrue((x) <= 255 ? show_ubyte((x)) : \
__knowntrue((x) <= 65535 ? show_uword((x)) : show_ulong(x))))

Although the inlined and overloaded functions available in C++ may reduce the need for a __knownconst() function, some microcontrollers do not have a practical C++ compiler available. Further, there are many situations where it would be worthwhile to test conditions which could be evaluated with zero execution-time cost, but it would not be worthwhile to test such conditions at run-time.

supercat9

Nemanja Trifunovic wrote:

I have yet to see such a case, at least with C++.

What would be your preferred way for initializing parallel arrays, or arrays that have to be parallel to enumerations? Abusing the preprocessor won't always allow one to write very nice source code (sometimes it's necessary to throw in garbage identifiers merely to keep the compiler from squawking) but how would you suggest doing something like the following if as much data as possible must be in read-only storage?

#include <stdio.h>

#define my_list \
X(foo) \
X(bar) \
X(quack) \
X(moo) \
X(meow) \
X(woof) \
X(bah) \
X(oink) \
/* Do-nothing line concatenated into above definition */

/* Define enumerations en_* for each item in my_list */
#define X(y) en_##y,
enum {my_list en_COUNT};
#undef X

/* Define st[] containing a string for each item in my_list */
#define X(y) #y,
const char *st[] = {my_list 0};
#undef X

void main(void)
{
printf("\n%d %s",en_foo,st[en_foo]);
printf("\n%d %s",en_moo,st[en_moo]);
printf("\n%d %s",en_bah,st[en_bah]);
printf("\n");
}

One could include the list twice in the source code--once to define enumeration types and once to define the strings--but that would run the risk of having the two copies get out of sync. One could also use some other utility to take a list of names and output C-compilable source to implement it. If sufficient RAM were available, one could generate st at runtime. On the other hand, if the preprocessor can do what needs to be done, why bring in some other tool?