thesofproject / sof

Sound Open Firmware
Other
514 stars 303 forks source link

[BUG] Checkpatch reports what appears to be a false error #3860

Open stolx opened 3 years ago

stolx commented 3 years ago

Describe the bug Checkpatch spawns an error on switch case statement in the situation when there is intentional fallthrough from case to default, indicated by COMPILER_FALLTHROUGH macro, defined in sof/compiler_attributes.h.

Consider following code:

    switch (n) {
    case 1:
        res = 1;
        break;
    case 2:
        COMPILER_FALLTHROUGH
    default:
        res = 2;
        break;
    }

Checkpatch will report an error:

> ERROR: spaces required around that ':' (ctx:VxE)
> +       default:

To Reproduce Run checkpatch on commit ee4539b

Reproduction Rate All the time

Expected behavior Checkpatch reports no errors

Impact False errors prevent automatic checks from passing during PR review.

Environment 1) Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).

Screenshots or console output

ERROR: spaces required around that ':' (ctx:VxE)
#44: FILE: src/include/sof/audio/format.h:191:
+       default:
               ^
plbossart commented 3 years ago

it's probably fooled because there's a missing ';' at the end of COMPILER_FALLTHROUGH.

Actually in this case there is no need for a fallthrough at all?

this could be

case 2: default: res = 2; break;

stolx commented 3 years ago

@plbossart I agree that fallthrough in this example is not needed. Even more so, case 2 is not needed, since

case 2:
default:
   res = 2;
   break;

is the effectively the same as

default:
   res = 2;
   break;

So I extended the example, added slightly more complicated "logic" to illustrate the error that I got:

    int res = 0;

    switch (a) {
    case 0:
        res += 1;
        break;
    case 1:
        res += 2;
        COMPILER_FALLTHROUGH
    default:
        res += 3;
        break;
    }

checkpatch on this code will spawn the same error

ERROR: spaces required around that ':' (ctx:VxE)
+       default:
               ^

new commit with example 6c7131c

lgirdwood commented 3 years ago

@stolx care to send a fix and remove case 2