What's wrong with my multilined macros?

Question

I have a function mainFunc which needs to call a several times another function process. A process has a lot of arguments but mainfunc in different calls change only two of them/ Instead of another it passes local variables which are defined and assigned before this calls to process. So I wrote a simple macro which substitutes local variables in calls to process:

#define DO_PROCESS(pred1, est1)     \
do                      \
{                       \
    process(pred1, est1, arg1, arg2, arg3); \
    ++id;                   \ 
    delete est1;                \
} while(0)

arg1, arg2, arg3 are local variables in mainFunc, so I hope my macro will just use them. In mainFunc:

int arg1, arg2, arg3;
arg1 = AssignFirst();
...
Pred* pred;
Est* est;
int estArg;
int predArg;

pred = new Pred(predArg);
DO_PROCESS(pred, new Est(estArg));
delete pred;

pred = new Pred(predArg2);
DO_PROCESS(pred, new Est(estArg2));
delete pred;

pred = new Pred(predArg3);
DO_PROCESS(pred, new Est(estArg3));
delete pred;
....

However I get C2059 and C2143 errors pointing to closing curly brace and semicolon around it respectively in last line of macro.

What's wrong with it??

What are C2059 and C2143 errors for those of us with the benefit of no access to MS Visual Studio? — Jonathan Leffler, Nov 07 '10 at 06:43
http://msdn.microsoft.com/en-us/library/0afb82ta.aspx, http://msdn.microsoft.com/en-us/library/t8xe60cf(VS.80).aspx — flashnik, Nov 07 '10 at 06:44
Usage of new in macro was not in production code, it appeared as a result of simplification to insert a sample here. The problem was in a space after a backslash, as pointed by Fabian Giesen. — flashnik, Nov 07 '10 at 06:47
Can you show the exact error messages - there are place holders in the error explanations, and we need to know exactly what it is trying to say. Also, have you looked at the output from the pre-processor to see whether that makes it clearer - because superficially, there isn't much wrong at the macro expansion syntax level, regardless of the memory leak/functional problems identified. — Jonathan Leffler, Nov 07 '10 at 06:51
`missing ";" before }`, `syntax error:}`. The problem is now solved - it was a space after one of the backslashes. — flashnik, Nov 07 '10 at 07:03
...And if I were your code reviewer, I'd send you back to your drawing board. I am sorry, but this kind of wrapping of non trivial code into macros is not helpful. It does spread confusion (a) at design time, as you could witness yourself (b) makes debugging, testing and all the other usefull things a hell.. — Paul Michalik, Nov 07 '10 at 12:28

Fabian Giesen · Accepted Answer · 2010-11-07T06:42:30.330

10

The most likely cause is that there's extra whitespace after one of the backslashes in the macro definition. Make sure that \ really is the last character on a line.

Oh, and your code will leak memory, since est1 is evaluated twice in your macro definition - it expands to process(pred1, new Est(...), ...) and then later delete new Est(...), which is not what you meant. Add something like Est *e = est1; before the process call and replace the remaining occurences of est1 in your macro with e to avoid this problem.

edited Nov 07 '10 at 06:42

answered Nov 07 '10 at 06:35

Fabian Giesen

3,231
16
16

1

You were right about a space after a backslash/ That was the error. – flashnik Nov 07 '10 at 06:42

score 9 · Answer 2 · answered Nov 07 '10 at 06:36

9

A macro is not like a function. Each occurrence of est1 is replaced by the literal text new Est(estArg). The last line of your macro becomes delete new Est(estArg).

answered Nov 07 '10 at 06:36

Ben Jackson

90,079
9
98
150

Yes, this a good point. I made this mistake when simplifying code to insert in question.. It was not the original one. – flashnik Nov 07 '10 at 06:48

score 3 · Answer 3 · answered Nov 07 '10 at 06:37

3

I'm not sure that this is it, but here's how this macro gets expanded:

pred = new Pred(predArg);
do {
    process(pred, new Est(estArg),arg1,arg2,arg3);
    ++id;
    delete new Est(estArg);
} while(0);
delete pred;

answered Nov 07 '10 at 06:37

Yuliy

17,381
6
41
47

score 3 · Answer 4 · answered Nov 07 '10 at 06:39

3

First off, why do ... while (0)? Why not just { code }? Second, sweet memory leak you've got there, DO_PROCESS(pred, new Est(estArg2)) substitutes to:

do
{
    process(pred, new Est(estArg2), arg1, arg2, arg3);
    ++id;
    delete new Est(estArg2);
} while(0)

answered Nov 07 '10 at 06:39

ta.speot.is

26,914
8
68
96

2

see http://stackoverflow.com/questions/154136/why-are-there-sometimes-meaningless-dowhile-and-ifelse-statements-in-cc-macros . – irrelephant Nov 07 '10 at 06:39

score 1 · Answer 5 · answered Nov 07 '10 at 06:34

1

You are using the macro var est1 in the delete statement as well.. that is likely the culprit!

answered Nov 07 '10 at 06:34

James

1,651
2
18
24

1

How can that be the culprit for the error he mentions? That's a syntax error he's getting. `delete new Whatever;` may be nonsensical in most contexts, but it's valid C++. – Fabian Giesen Nov 07 '10 at 06:44
Yup, realized that after reading the other response and up-voted that one. – James Nov 08 '10 at 17:05

score 1 · Answer 6 · answered Nov 07 '10 at 06:41

1

The macro expands (in part) to:

process(pred, new Est(estArg), arg1, arg2, arg3);
id++;
delete new Est(estArg);

That isn't going to do what you want it to do - a macro is not a function. It leaks memory.

answered Nov 07 '10 at 06:41

Jonathan Leffler

730,956
141
904
1,278

What's wrong with my multilined macros?

6 Answers6