CODEWITHSUNDEEP
cavravr-gcc
hvguy
hvguy

Reputation: 41

C macro compiler warnings

I've defined a macro, using input from a previous question I asked here. The macro is intended to either set, clear, or check a GPIO pins state. The macro works as expected however a problem shows up when compiling. I get compiler warnings anywhere it's used:

Warning right-hand operand of comma expression has no effect

when I use the macro like this:

#define ON  1
#define OFF 2
#define ENA 3
#define OUT_3(x) (x==ON) ? (PORTJ.OUTSET=PIN2_bm) : (x==OFF) ? (PORTJ.OUTCLR=PIN2_bm) : (x==ENA) ? (PORTJ.DIRSET=PIN2_bm) : (PORTJ.DIRCLR=PIN2_bm)
#include <avr/io.h>

if (something) OUT_3(ENA);

However, if I do this:

if (something) {OUT_3(ENA);}

I no longer get the warnings.

Why is there a difference? How should I alter the macro to prevent this scenario?

Additionally this invokes the warning:

int i=0;
if (something) i=1, OUT_3(ENA);

However this does not:

int i=0;
if (something) OUT_3(ENA), i=1;

My understanding of comma separated expressions is clearly a bit off. How is the compiler seeing this? I looked at several other questions similar to this but still do not fully understand the difference.

Upvotes: 0

Views: 302

Answers (2)

user694733
user694733

Reputation: 16043

That macro is nasty for several reasons:

  1. Macro parameters should always be surrounded with parenthesis to avoid potential problems with operator precedence. Change (x==ON) to ((x)==ON).
  2. Nested ternary operations should be surrounded with parenthesis to make execution order obvious. Change a ? b : c ? d : e to a ? b : (c ? d : e).
  3. Complete macro should be surrounded with parentheses #define MACRO (...) or do-while-zero loop #define MACRO do {...} while(0) to avoid possible problems with operator precedence. More below.

  4. Ternary operator is not really useful here as you are not using it's return value. You should rather use regular if or switch statements. This is where previously mentioned do-while-zero loop becomes handy:

    #define OUT_3(x) \
    do { \
        if((x) == ON)       { PORTJ.OUTSET = PIN2_bm; } \
        else if((x) == OFF) { PORTJ.OUTCLR = PIN2_bm; } \
        else if((x) == ENA) { PORTJ.DIRSET = PIN2_bm; } \
        else                { PORTJ.DIRCLR = PIN2_bm; } \
    } while(0)

  5. But is macro really even necessary? You could use inline function instead, and get rid of all macro weirdness:

    static inline void OUT_3(x_type x) {
    if(x == ON)       { PORTJ.OUTSET = PIN2_bm; }
    else if(x == OFF) { PORTJ.OUTCLR = PIN2_bm; }
    else if(x == ENA) { PORTJ.DIRSET = PIN2_bm; }
    else              { PORTJ.DIRCLR = PIN2_bm; }
}

With these changes, your error will likely go away, and your code is much easier to read.

Upvotes: 5

A.K.
A.K.

Reputation: 989

I can't reproduce your issue. This code works without any problem

main.cpp

#include <stdio.h>
enum type{ON, OFF, ENA};

#define OUT_3(x) (x==ON) ? (printf("ON\n")) : (x==OFF) ? (printf("OFF\n")) : (x==ENA) ? (printf("ENA\n")) : (printf("OTHER\n"))

int main(){
  int a = 2;
  if(a == 1) OUT_3(ON);
  if(a == 2) OUT_3(OFF);
  if(a == 3) OUT_3(ENA);

  return 0;
}

Compiled with

gcc -Wall -O0 -g -o main main.c

Could you please show how the definitions of ON, OFF, ENA and the Port definitions look like?

Upvotes: 1

Related Questions