3

I write an example to show my problem, here it is to use nested macros to make calls to check the number is positive and odd. I know this is redundant to use this call, but as I said previously, it just show that if using macro in nested way, there is some problem:

#include <stdio.h>
#include <stdlib.h>

#define num_is_positive_odd(num)                           \
({                                                         \
    int __rc = 0;                                          \
    int __num = (num);                                     \
                                                           \
    printf("%s:%d: check linenum\n", __func__, __LINE__);  \
    if (num_is_positive(__num) && num_is_odd(__num))       \
        __rc = 1;                                          \
    __rc;                                                  \
 })

#define num_is_positive(num)                  \
({                                            \
    int __rc = 0;                             \
    int __num = (num);                        \
                                              \
    if (__num > 0) {                          \
        printf("%s: number %d is positive\n", \
               __func__, __num);              \
        __rc = 1;                             \
    }                                         \
    __rc;                                     \
 })

#define num_is_odd(num)                       \
({                                            \
    int __rc = 0;                             \
    int __num = (num);                        \
                                              \
    if (__num / 2) {                          \
        printf("%s: number %d is odd\n",      \
               __func__, __num);              \
        __rc = 1;                             \
    }                                         \
    __rc;                                     \
 })



int main()
{
    int num = 4;

    if (num_is_positive_odd(num++))
        printf("%s: number %d is positive odd\n", __func__, num);

    exit(0);
}

When compile it using command: gcc -Wunused-variable chk_comps.c

it shows error:

chk_comps.c: In function ‘main’:
chk_comps.c:7:9: warning: unused variable ‘__num’ [-Wunused-variable]
     int __num = (num);                                     \
         ^
chk_comps.c:47:9: note: in expansion of macro ‘num_is_positive_odd’
     if (num_is_positive_odd(num++))
         ^

`

Could somebody help explain why and how to fix it? thanks.

rici
  • 234,347
  • 28
  • 237
  • 341
  • That doesn't expand out to valid C code. – John3136 Jun 08 '18 at 03:36
  • 1
    This is using a GCC extension called [statement expressions](https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/Statement-Exprs.html#Statement-Exprs) — so the rules are specific to GCC (and probably Clang emulating GCC). I can't help but feel you'd be better off using static inline functions rather than macros piled upon macros with the `__num` name (mis)used and (re)used all over the place. – Jonathan Leffler Jun 08 '18 at 03:39
  • 1
    Note that you should not create function, variable or macro names that start with an underscore, in general. [C11 §7.1.3 Reserved identifiers](https://port70.net/~nsz/c/c11/n1570.html#7.1.3) says (in part): — _All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use._ — _All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces._ See also [What does double underscore (`__const`) mean in C?](https://stackoverflow.com/a/1449301/15168) – Jonathan Leffler Jun 08 '18 at 03:42
  • thanks for the details. – user6804435 Jun 08 '18 at 07:11
  • Why on earth did you use macros instead of functions? You have at least 10 operator precedence bugs in there. For the love of obfuscation, rewrite this to functions. Hint: if you did this just to get the `__func__` of the caller, then just do some wrapper macro like `#define num_is_odd(num) number_is_odd(num, __func__)` where you have `bool number_is_odd (int num, const char* caller);`. – Lundin Jun 08 '18 at 12:53

3 Answers3

3

You do the following macro expansion:

if (num_is_positive(__num) && num_is_odd(__num)) 

Both of those expand to block expressions which include the declaration:

int __num = (num);  

Where num is the macro parameter for the inner macro. Since the actual macro argument is __num, the resulting replacement text is

int __num = (__num);  

The scope of a C declaration starts as soon as the declaration is complete, and before the definition. So both __num in that declaration refer to the same local variable (which is therefore initialised to an uninitialised value). Meanwhile, the __num in the outer block is completely shadowed by the __nums in the inner blocks, and is, as the compiler notes, unused. Had you specified -Wall instead of just enabling one warning, the compiler probably would have warned you about the uninitialised initialisation, too, which might have been another clue.

By the way, names starting with two underlines are reserved; you are not allowed to create them. So your macros exhibit undefined behaviour. That is not your immediate problem, but since you'll have to change the names anyway to impose hygiene, you might as well do it right.

Fix this by using static inline functions instead of macros. Seriously. Functions have predictable scope, unlike macros whose expansions are, as you can see, unhygienic. They require less thought on your part, are easier to debug, read and understand, and are not a cycle slower.

rici
  • 234,347
  • 28
  • 237
  • 341
3

This is using a GCC extension called statement expressions — so the rules are specific to GCC (and probably Clang emulating GCC).

If you run gcc -E, you can see that the raw output for main() (with the void added by me) is:

# 41 "gccm43.c"
int main(void)
{
    int num = 4;

    if (({ int __rc = 0; int __num = (num++); printf("%s:%d: check linenum\n", __func__, 45); if (({ int __rc = 0; int __num = (__num); if (__num > 0) { printf("%s: number %d is positive\n", __func__, __num); __rc = 1; } __rc; }) && ({ int __rc = 0; int __num = (__num); if (__num / 2) { printf("%s: number %d is odd\n", __func__, __num); __rc = 1; } __rc; })) __rc = 1; __rc; }))
        printf("%s: number %d is positive odd\n", __func__, num);
    return 0;
}

which, when manually formatted (a variation on the theme of 'purgatory'), might look like:

# 41 "gccm43.c"
int main(void)
{
    int num = 4;

    if (({ int __rc = 0;
           int __num = (num++);
           printf("%s:%d: check linenum\n", __func__, 45);
           if (({ int __rc = 0;
                  int __num = (__num);
                  if (__num > 0)
                  {
                      printf("%s: number %d is positive\n", __func__, __num);
                      __rc = 1;
                  }
                  __rc;
                }) &&
                ({ int __rc = 0; 
                   int __num = (__num);
                   if (__num / 2)
                   {
                       printf("%s: number %d is odd\n", __func__, __num);
                       __rc = 1;
                   }
                   __rc;
                 }
               ))
           __rc = 1;
           __rc;
         }
       ))
        printf("%s: number %d is positive odd\n", __func__, num);
    return 0;
}

The lines int __num = (__num); are problematic; you are initializing the variable with itself, which doesn't really do a good job (the value is indeterminate before and after the initialization). You also used (__num / 2) to determine whether __num is odd, which is an odd way to detect oddness; you should use (__num % 2).

It is also now evident why the compiler warns that (one of the) __num (variables) is not used. The outer declaration assigns num++ to __num, but the initialized variable is never used because the inner occurrences of int __num = (__num); refer to themselves and not to the outer __num, so it isn't used.

You'd do better with static inline functions — something like this:

#include <stdio.h>
#include <stdlib.h>

static inline int num_is_positive(int num)
{
    int rc = 0;

    if (num > 0)
    {
        printf("%s: number %d is positive\n", __func__, num);
        rc = 1;
    }
    return rc;
}

static inline int num_is_odd(int num)
{
    int rc = 0;

    if (num % 2)        // BUG fixed
    {
        printf("%s: number %d is odd\n", __func__, num);
        rc = 1;
    }
    return rc;
}

static inline int num_is_positive_odd(int num)
{
    int rc = 0;

    printf("%s:%d: check linenum\n", __func__, __LINE__);
    if (num_is_positive(num) && num_is_odd(num))
        rc = 1;
    return rc;
}

int main(void)
{
    int num = 4;

    if (num_is_positive_odd(num++))
        printf("%s: number %d is positive odd\n", __func__, num);

    return 0;
}
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • thank you, I guess the big problem is bad function arrangement. in our code, if the above logic is defined in header file as static function, it maybe not used in some other c files, then it would cause another compile problem. – user6804435 Jun 08 '18 at 07:10
  • 1
    If you use `static inline` (and C99 or C11), then it won't matter if the functions are defined in a header and are not used everywhere. The unused functions will simply not be instantiated. If you make them 'plain `static`', then you'll have unused functions and the compiler will probably complain about it, and rightly so. That's why I used `static inline`. – Jonathan Leffler Jun 08 '18 at 07:21
  • (And of course, in any header use header guards: `#ifndef FOO_H #define FOO_H ... #endif`.) – Lundin Jun 08 '18 at 12:56
1

To complete both Jonathan Leffer's answer and rici's one: if your actual code requires statement expressions and cannot be done by static inline functions (this is unlikely and not the case in the functions you have shown in your question), you should consider generating unique identifiers in your macros (to make them more hygienic). A possible way (specific to GCC!) is to use cpp concatenation with __COUNTER__ like this:

#define num_is_positive_count(num,Count)     \
({                                           \
    int rc_##Count = 0;                      \
    int num_##Count = (num);                 \
                                             \
    if (num_##Count > 0) {                   \
        printf("%s:number %d is positive\n", \
               __func__, num_##Count);       \
        rc_##Count = 1;                      \
    }                                        \
    rc_##Count;                              \
 })

(BTW, I am avoiding identifiers starting with _)

Then you need a double indirection

 #define num_is_positive_count2(num,Count) \
    num_is_positive_count(num,Count)
 #define num_is_positive(num) num_is_positive_count2(num,__COUNTER__)

But you'll better use static inline functions when you can. This is usually the case!

BTW Both __COUNTER__ and statement expressions are GNU extensions (accepted by GCC & Clang), outside of the C11 standard n1570. In some cases (not in yours), you might use (if you don't want to depend on the GNU-ism __COUNTER__...) the standard __LINE__ instead of the GNU __COUNTER__ and adopt the convention to invoke your macros one per line (see also this).

Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547
  • Using `__LINE__` for hygiene isn't very effective because all macro expansions inside a macro expansion have the same source line number. It would not have worked in OP's code, for example. – rici Jun 08 '18 at 12:55
  • But if every macro invocation was on its own line, it might work – Basile Starynkevitch Jun 08 '18 at 12:57
  • Not if the macro expansion includes another macro expansion which uses the same hack. In OP's code, the outer macro expands two inner macros, and those both will have the same line number no matter how you write it. – rici Jun 08 '18 at 13:04