c, if statement doesn't work properly

Question

I did this simple program but my if statement doesn't work properly. Here my code:

void SCRIVI_RUOTA(int s, int i, int c)
 {
    _CONFIG_DRIVE_PORT_OUTPUT_VALUE(C, (segK|segL|segM), 0, (PORT_SRE_SLOW | PORT_DSE_HIGH));  // this switch off all my LEDS

s=0;
i=0;
c=0;
    if(s==1)
        _CONFIG_DRIVE_PORT_OUTPUT_VALUE(C, (segK), (segK), (PORT_SRE_SLOW | PORT_DSE_HIGH));  // this switch on my LED N° 1
    if(i==1)
        _CONFIG_DRIVE_PORT_OUTPUT_VALUE(C, (segM), (segM), (PORT_SRE_SLOW | PORT_DSE_HIGH));  // this switch on my LED N° 2
    if(c==1)
        _CONFIG_DRIVE_PORT_OUTPUT_VALUE(C, (segL), (segL), (PORT_SRE_SLOW | PORT_DSE_HIGH));  // this switch on my LED N° 3

}

well, I can put s, i, c, equals to 0 or 1 but the if statement is always execute and my LEDS turn on in any case.

if I delete the switch on command inside the if statement the LED doesn't turns on (and this means that there are not other functions that make conflicts forcing the LEDs to high). if I put an else after the if statements that switch OFF the LEDs all the LEDs switch OFF. It sounds like if the if and else statement doesn't exist and the last command on the LED is execute. Is it possible exist functions or macros or something else that ignore the if statement?

you set s,i,c = 0. so you ignore all values passed in. Why are you doing that? — pm100, Apr 13 '18 at 22:26
that's just a test, in real program I don't put s, i, c =0 here but the variables are passed by the function — Simone Del Gobbo, Apr 13 '18 at 22:30
try doing this `if(s==1) {_CONFIG.....}`. Ie wrap {} around the condition statement. That macro might be doing something wierd — pm100, Apr 13 '18 at 22:32
Also, name the macro without the leading underscore. Identifiers with an underscore followed by an uppercase letter are reserved for use by the implementation — Peter, Apr 13 '18 at 22:42

jxh · Answer 1 · 2018-04-14T00:05:24.487

3

It is likely your macro expands into multiple statements, and is not wrapped with do .. while(0).

Surround the macro with {} in your if statements.

if(s==1) {
    _CONFIG_DRIVE_PORT_OUTPUT_VALUE(...);
}
//...

As noted in the comments, the original macro can be considered broken, and should be fixed.

Assuming the macro is defined like:

#define _CONFIG_DRIVE_PORT_OUTPUT_VALUE(ref, pins, value, chars) \
    GPIO##ref##_PIDR &= ~(pins);                                 \
    GPIO##ref##_PDOR = ((GPIO##ref##_PDOR & ~(pins)) | (value)); \
    GPIO##ref##_PDDR |= (pins);

It should be changed to be this instead:

#define _CONFIG_DRIVE_PORT_OUTPUT_VALUE(ref, pins, value, chars) \
    do {                                                         \
    GPIO##ref##_PIDR &= ~(pins);                                 \
    GPIO##ref##_PDOR = ((GPIO##ref##_PDOR & ~(pins)) | (value)); \
    GPIO##ref##_PDDR |= (pins);                                  \
    } while(0)

If the macros were fixed, then your original code would work unmodified.

edited Apr 14 '18 at 00:05

answered Apr 13 '18 at 22:31

jxh

69,070
8
110
193

1

.... or change the macro to wrap everything in `do { .... }while (0)`. – Peter Apr 13 '18 at 22:40
See [Why use apparently meaningless do-while and if-else statements in macros?](https://stackoverflow.com/q/154136/12711) – Michael Burr Apr 13 '18 at 22:43
@Peter: I doubt the asker authored the macros, but yes, multi-statement macros should be wrapped. – jxh Apr 13 '18 at 23:02
@jxh - I wouldn't exclude the possibility that she authored the macros. My comment was more along the lines of something she could report to the author of the macros as a flaw. – Peter Apr 13 '18 at 23:05
Yes - if the macros aren't wrapped, they're broken. Whoever is responsible for them should fix them. – Michael Burr Apr 13 '18 at 23:11
@Peter: Simone has a beard. ;-) – Jeff Learman Apr 13 '18 at 23:45
1

@JeffLearman - so does the lady who lives next door to me :-) – Peter Apr 13 '18 at 23:52
@MichaelBurr: Answer updated, but many style guides mandate that the body of a `if`, `while`, etc. should always have `{` .. `}`. – jxh Apr 14 '18 at 00:52
The thing is, it costs nothing to wrap the macros and then they work whether or not the user follows the style guide. I think it's important because the macro is designed to hide the fact that there are multiple statements, so they should be designed to work exactly like a single statement. And as far as style guides go, at least one important project's style guide explicitly forbids putting braces around single lines controlled by an `if` or `else` (the Linux Kernel style). I don't agree with that guidance, but I'm not a Linux Kernel contributor, so they don't really care what I think. – Michael Burr Apr 14 '18 at 04:12
@MichaelBurr: I am not disputing the benefits of wrapping macros with multiple statements. I believe just as you do. I am just pointing out that there are mitigations available that would have avoided the issue encountered by the OP. – jxh Apr 14 '18 at 04:48

c, if statement doesn't work properly

1 Answers1