CODEWITHSUNDEEP
cgoto
George Wu
George Wu

Reputation: 68

wrapping goto labels in parenthesis

so i have a marco function like so:

#define PROPOGATE_METHOD(status, function, propogationMethod) \
     status = function; \
     if(status != eSuccess)\
     {\
        propogationMethod; \
     }

So like any good developer would do, I want to wrap each of the parameters as such :

#define PROPOGATE_METHOD(status, function, propogationMethod) \
     (status) = (function); \
     if((status) != eSuccess)\
     {\
        (propogationMethod); \
     }

But if I call this macro function with a goto or return, I get an error (expecting expression before goto). i.e. PROPOGATE_METHOD(status, functionCall(), goto Error;);

Thoughts on working around this? I was thinking of moving the goto into the macro function, and wrapping around the label, but that throws another error : expected identifier or ‘*’ before ‘(’ token

Upvotes: 0

Views: 393

Answers (3)

klutt
klutt

Reputation: 31376

So like any good developer would do, I want to wrap each of the parameters as such

@StoryTeller had a good response to this in comment section. " A good developer understands why the suggestion exists, what problem it solves, and most importantly, when it's not applicable. Blindly doing something is not good development.".

Another good applicable quote is "Blindly following best practices is not best practice".

Here, it really seems like you're adding parenthesis because someone said "it's a good thing to put parenthesis around the arguments". Not because of any valid purpose in this particular case.

Skip the macro

I don't really see the purpose with this macro. TBH, it looks like you're showing off, but macros like this are very likely to cause hard traced bugs. If it's just to save some lines, you can actually make a somewhat decent oneliner of this without any macro.

if((status = foo()) != eSuccess) goto Error;

or

if((status = foo()) != eSuccess) return x;

or

if((status = foo()) != eSuccess) bar();

In many cases, I'd prefer making those on two or three lines. But the above is not so bad. And I would definitely say that it's better than the macro. Just remember the extra parenthesis around status = foo(). If forgetting this is a big concern, you could do something like this:

int foo_wrapper(int *status) { return *status = foo(); }
...
if(foo_wrapper(&status) != eSuccess) goto Error;

or even:

int status_assign(int *dest, int src) { return *dest = src; }
...
if(status_assign(&status, foo()) != eSuccess) goto Error;

On the other hand, it shouldn't be a problem, because if you compile with -Wall, which you should, you will get this:

warning: suggest parentheses around assignment used as truth value

Personally, I don't think it's extremely important with braces when it's single statements, but if you want a oneliner with braces, well just do:

if((status = foo()) != eSuccess) { goto Error; }

Some will like it. Some will not, but it's not the most important question in the world. But I would prefer any of the above before the macro you're suggesting.

Compare these:

PROPOGATE_METHOD(status, foo(), goto Error;);
if((status = foo()) != eSuccess) goto Error;

When compared side by side, I cannot really see that the macro accomplishes anything at all. It doesn't make anything clearer or safer. Without the macro, I can see EXACTLY what's happening and I don't need to wonder about that. Macros have their uses, but as far as I can see here, this is not one of them.

From comments below

I understand. i'm going through and refactor a code base. i prefer not to littler the code base with these if statements and prefer the macro statement because

I can understand that, but I would really encourage you to reconsider. At least if you're allowed to do it. If I were to refactor that code base, getting rid of that macro would have pretty high priority. After all, what refactoring is, is to rewrite code so it becomes better with focus on design and readability. If you don't want to do it, put parenthesis around status and function and then leave it. It will not make it good, but it will not cause any harm either.

I would not use this expression if it were you who had written that macro, but since it's not you, I can use it. "Fixing" that macro is really polishing a turd. No matter what you do, it will never shine, and it will always be a turd.

Upvotes: 3

Petr Skocik
Petr Skocik

Reputation: 60068

The goto statement (goto label;) isn't an expression, so you cannot parenthesize it (and neither is goto label without the ;, which isn't even a separately recognizable construct in C's syntax).

And even if you passed something that you can parenthesize (e.g., longjmp(jbuf,1)), there isn't much of a point in parenthesizing it in this context ({ HOOK; }).

Now if you expanded it in a context like HOOK_EXPR * 2, then parentheses would be useful to force HOOK_EXPR to group tighter than * (imagine you passed 3+4 as HOOK_EXPR), but in this context you don't need them.

Upvotes: 1

ilkkachu
ilkkachu

Reputation: 6527

Disregarding if that macro is useful or confusing, and the merits of goto, consider what the parens inside a macro/define are for. If you have say, this:

#define FOO a + b
...
int y = x * FOO;

what you end up with, is y = x * a + b (because it's just text replacement, not a real variable), which is the same as y = (x * a) + b. Hence, putting parens around (a + b) in FOO fixes that.

This, of course has a similar problem (both inside x and outside the macro), with a similar solution:

#define FOO2(x) x * 123
...
int y = FOO2(a + b);

Now, you have

#define BAR(x) { x };

is there a similar problem there? What should x include that the parenthesis would remove a similar problem stemming from operator precedence? I don't really see such an issue, in a way, the braces already work to protect the x part from the code surrounding the macro. Adding the parens has just the effect of forcing x to be an expression, instead of a full statement.

Upvotes: 1

Related Questions