The right hand operand of a logical operator || has persistent side effects because of calling function (MISRA- C 2012Rule 13.5)

Question

The right hand operand of a logical operator || has persistent side effects because of calling function detectError().

if ( ( detect() == VALID ) || 
         ( detectError() == INVALID ) )
    {
        up( a,b );  
      }

typedef enum
{

C;

}E_name;

typedef struct
{
 
 E_name be:4;
  
}S_name;
S_name name;

persistent_side_effect: Expression name.be = C has persistent side effect: modifying non-local object okay.be = C.

sint16 detectError(void)
{
name.be = C;
}

I was able to solve logical operator &&, is there a solution for || operator?

A solution for what problem? Are you asking this: https://stackoverflow.com/questions/54114895/misra-13-5-question-about-non-compilant-example ? How did you "solve" it for the `&&` operator? What is `detectError()`? What is `detect)`? — Jabberwocky, Dec 13 '21 at 12:42
Do not just write numbers as if people are supposed to know what they are. If you are citing a MISRA rule, state in your question that it is a MISRA rule. And preferably include the year of the MISRA edition. — Eric Postpischil, Dec 13 '21 at 12:46
The code you have shown is incomplete. No connection between `detectError()` and `name.be = C;` is visible in the code shown, but the error message suggests the MISRA checker is complaining that `detectError()` somehow includes execution of `name.be = C;`. In which case, (a) update the question to provide a [mre], and (b) explain why `detectError` causes a change to `name.be`. A name like `detectError` suggests the function is **only** going to check something and report a result, not that it is going to change something. Why is it changing anything? — Eric Postpischil, Dec 13 '21 at 12:53
The fact that `detectError` apparently changes something is in fact the reason for the MISRA rule: It is a surprise to the reader, so it should not be there. — Eric Postpischil, Dec 13 '21 at 12:54
```name.be = C``` is inside the function ```detectError()```. I hope this helps. I'm sorry, I tried to make it as simple as possible. — Sharon Deborah, Dec 13 '21 at 14:58
Do you want `name.be = C` to be always executed when this test is performed, or do you want it to be executed only if `detect()` does not return `VALID`? — Eric Postpischil, Dec 13 '21 at 15:05
. The right hand operand of a logical operator && has persistent side effects. ```if(var == 3 && var2 ==7){ }``` Solution: ```if(var ==3){ if(var1==7){ } }``` @Jabberwocky — Sharon Deborah, Dec 13 '21 at 17:52
It's an OR operand, so it's either that or this, but in this case the problem seemed to the the readability of the code, the issue was solved when I assigned the functions ```detect()``` and ```detectError()``` to a variable. @Eric Postpischil — Sharon Deborah, Dec 13 '21 at 18:05

Andrew · Accepted Answer · 2021-12-13T14:34:00.793

3

Surely the simplest work around for this is:

whateverType detectFlag1 = detect();
whateverType detectFlag2 = detectError();

if ( ( detectFlag1 == VALID ) || ( detectFlag2 == INVALID ) )
{
   up( a,b );  
}

Simple, clear code, with no potential side effects?

edited Dec 13 '21 at 14:34

answered Dec 13 '21 at 14:24

Andrew

2,046
1
24
37

3

That changes the code so the side effects of `detectError` are always realized, which might or might not be desired. – Eric Postpischil Dec 13 '21 at 14:42
This solves the issue, thanks! – Sharon Deborah Dec 13 '21 at 17:49

score 2 · Answer 2 · answered Dec 14 '21 at 10:31

Generally, code with MISRA-C quality concerns needs to be deterministic and there should exist at least one use-case where some part of the code gets executed (code coverage). In this case there is no telling if detectError() gets called or not, which may or may not be problematic depending on if that function contains any side effects.

Also, common sense doesn't sit well with "if detect is valid or detect error is invalid". What's that even supposed to mean, if detect failed but you couldn't detect errors, then wouldn't that leave your program in an undefined state?

Of course I have no idea what these functions are doing, but maybe at least consider better identifier naming. Maybe "detect error" should be called "get last error" or such.

Assuming the code is correct, then you can rewrite it in a clearer but otherwise equivalent manner like this:

if(detect() == VALID)
{
  up(a, b);
}
else if(detectError() == INVALID)
{
  up(a, b);
}
else
{
  ; // possibly handle this scenario or leave it blank
}

Note that the else is mandatory as per MISRA-C defensive programming/self-documenting code.

Other concerns:

Using bit-fields in a MISRA-C application is a very bad idea. MISRA-C doesn't prohibit them, but they are generally non-portable and poorly standardized, so they don't belong in a deterministic or portable program.
Don't come up with home-made garage standards for integers such as sint16. Use standard C sint16_t from stdint.h instead. If you are stuck with C90 then make typedefs corresponding to the stdint.h names.

The right hand operand of a logical operator || has persistent side effects because of calling function (MISRA- C 2012Rule 13.5)

2 Answers2