c:S1862 Chain of if/else doesn't consider function parameter mutations

Rules and Languages Report False-positive / False-negative...
1
  • C
  • S1862
  • It flags the conditions as being repeated leading to dead code, but doesn’t consider that the variable being compared mutates as a function parameter
  • SonarQube Server Developer Edition?
    SonarQube Cloud?

We are on an microcontroller with limited RAM & stack size, so we want to be able to re-use variables whenever possible.

status_t status = STATUS_OK;

if (readStatus(&status, 0) != READ_OK)
  printf("Couldn't read status 1");
else if (status != STATUS_OK)
  printf("Status 1 Error");
else if (readStatus(&status, 1) != READ_OK)
  printf("Couldn't read status 2");
else if (status != STATUS_OK)
  printf("Status 2 Error");
...
1 Like
2

Let me start that I’m both not a C-expert, neither employed by Sonar. I can see why this is strictly speaking a FP, and I think you have valid reasons to use for this construct.

That being said, I think, in most cases you would discourage this construct, don’t you think?

4

Yes, it may not be the most readable code, but in cases of limited RAM and stack size, you have to balance readability with not allocating variables unnecessarily.

The chaining of the if-else statements could be avoided by some other methods, like surrounding everything in a loop then using a break statement, or using a goto statement, but I would argue that those are also undesirable for readability as well.

Also this is code provided by a vendor, so I would like to avoid modifying it to avoid complicating merging in any future updates.

In any case, this is not dead code, which is what this rule attempts to flag, so my opinion is that this is a false positive.

5

As stated, I get your reasoning, and I would not advise you to change this code (given your constraints).

Let’s hope that someone of Sonar, responsable for the C-stack can share their thoughts too.

7

Thanks for your report!

In general I would advise to test whether the readable version generates less optimal assembly than the less readable version. In my experience, compilers are quite good at optimizing and reusing the stack (or a register) when a variable is no longer used. I’ve seen this happen even when the lifetime of the variable has not ended by the rules of the language.

Putting it in a loop and using a format string like printf("Status %d Error", ...); might reduce the size of the binary. This format string stores fewer string literals in the binary. I personally don’t mind a break in a loop or I move the block to a new function so I can return in the loop.

I agree this is a good reason not to modify the code. You can mark the issue as a false positive so it is no longer shown.

I agree this is a false positive and I’ve created a CPP-6611.