MISRA2012 Rule 12.1 Extra parentheses recommended-CodePudding

I'm trying to fix the compliance of my code to misra C. During the static analysis, I had this violation:

Rule 12.1: Extra parentheses recommended. A conditional operation is the operand of another conditional operator.

The code is:

if (CHANNEL_STATE_GET(hPer, channel) != CHANNEL_STATE_READY)
{
    retCode = ERROR;
}

where CHANNEL_STATE_GET is a macro as follow:

#define CHANNEL_STATE_GET(__HANDLE__, __CHANNEL__)\
  (((__CHANNEL__) == CHANNEL_1) ? (__HANDLE__)->ChannelState[0] :\
   ((__CHANNEL__) == CHANNEL_2) ? (__HANDLE__)->ChannelState[1] :\
   ((__CHANNEL__) == CHANNEL_3) ? (__HANDLE__)->ChannelState[2] :\
   ((__CHANNEL__) == CHANNEL_4) ? (__HANDLE__)->ChannelState[3] :\
   ((__CHANNEL__) == CHANNEL_5) ? (__HANDLE__)->ChannelState[4] :\
   (__HANDLE__)->ChannelState[5])

Do you have any idea to solve this violation?

BR, Vincenzo

CodePudding user response：

There's several concerns here, as far as MISRA C is concerned:

There's various rules saying that macros and complex expressions should be surrounded by parenthesis, and that code shouldn't rely on the C programmer knowing every single operator precedence rule. You can solve that by throwing more parenthesis on the expression, but that's just the top of the iceberg.
The ?: operator is considered a "composite operator" and so expressions containing it are considered "composite expressions" and come with a bunch of extra rules 10.6, 10.7 and 10.8. Meaning that there is a lot of rules regarding when and how this macro may be mixed with other expressions - the main concerns are implicit, accidental type conversions.
The use of function-like macros should be avoided in the first place.
Identifiers beginning with multiple underscores aren't allowed by MISRA C since the C language reserves those for the implementation.

The easier and recommended fix is just to forget about that macro, since it will just cause massive MISRA compliance headache. Also at a glance, it looks like very inefficient code with nested branches. My suggested fix:

In case hPer happens to be a pointer to pointer (seems like it), then dereference it and store the result in a plain, temporary pointer variable. Don't drag the nasty pointer to pointer syntax around across the whole function/macro.
Replace this whole macro with a (inline) function or a plain array table look-up, depending on how well you've sanitized the channel index.
Ensure that CHANNEL_1 to CHANNEL_5 are adjacent integers from 0 to 4. If they aren't, use some other constant or look-up in between.

A MISRA compliant re-design might look like this:

typedef enum
{
  CHANNEL_1,
  CHANNEL_2,
  CHANNEL_3,
  CHANNEL_4,
  CHANNEL_5
} channel_t;

// state_t is assumed to be an enum too

state_t CHANNEL_STATE_GET (const HANDLE* handle, channel_t channel)
{
  if((uint32_t)channel > (uint32_t)CHANNEL_5)
  {
    /* error handling here */
  }

  uint32_t index = (uint32_t)channel;
  return handle[index];
}

...

if (CHANNEL_STATE_GET(*hPer, channel) != CHANNEL_STATE_READY)

If you can trust the value of channel then you don't even need the function, just do a table look-up. Also note that MISRA C encourages "handle" in this case to be an opaque type, but that's a chapter of its own.

Note that this code is also assuming that HANDLE isn't a pointer hidden behind a typedef as in Windows API etc - if so then that needs to be fixed as well.

CodePudding user response：

Note (as more or less implied by Lundins comment....), I answer more about how to approach MISRA findings (and those of a few other analysis tools I suffered from ....).

I would first try to get a better angle on what the finding is actually describing. And with a nested structure like shown, that takes some re-looking. So ...

I would apply indentation, just to make life easier while editing and then, well, add some more () in inviting places, e.g. in this case so as to enclose each x?y:z into one pair.

#define CHANNEL_STATE_GET(__HANDLE__, __CHANNEL__)\
  (        ((__CHANNEL__) == CHANNEL_1) ? (__HANDLE__)->ChannelState[0] :\
    (      ((__CHANNEL__) == CHANNEL_2) ? (__HANDLE__)->ChannelState[1] :\
      (    ((__CHANNEL__) == CHANNEL_3) ? (__HANDLE__)->ChannelState[2] :\
        (  ((__CHANNEL__) == CHANNEL_4) ? (__HANDLE__)->ChannelState[3] :\
          (((__CHANNEL__) == CHANNEL_5) ? (__HANDLE__)->ChannelState[4] :\
                                          (__HANDLE__)->ChannelState[5]  \
          )                                                              \
        )                                                                \
      )                                                                  \
    )                                                                    \
  )

This is to address what the quoted finding is about.
I would not feel bad about sprinkling a few more around e.g. each CHANNEL_N.

(I admit that I did not test my code against a MISRA checker. I try to provide an approach. I hope this fixes the mentioned finding, possibly replacing it with another one.... MISRA in my experience is good at that.... I do not even expect this to solve all findings.)

CodePudding user response：

Other comments and answers are suggesting replacing the cumbersome CHANNEL_STATE_GET macro with a much simpler array lookup, and I strongly agree with that recommendation.

It's possible, though, that the definitions of CHANNEL_1 through CHANNEL_5 are not under your control, such that you can't guarantee that they're consecutive small integers as would be required. In that case, I recommend writing a small function whose sole job is to map a channel_t to an array index. The most obvious way to do this is with a switch statement:

unsigned int map_channel_index(channel_t channel)
{
    switch(channel) {
        case CHANNEL_1: return 0; break;
        case CHANNEL_2: return 1; break;
        case CHANNEL_3: return 2; break;
        case CHANNEL_4: return 3; break;
        case CHANNEL_5: return 4; break;
        default:        return 5; break;
    }
}

Then you can define the much simpler

#define CHANNEL_STATE_GET(handle, channel) \
        ((handle)->ChannelState[map_channel_index(channel)])

Or, you can get rid of CHANNEL_STATE_GET entirely by replacing

if(CHANNEL_STATE_GET(hPer, channel) != CHANNEL_STATE_READY)

with

if((*hPer)->ChannelState[map_channel_index(channel)] != CHANNEL_STATE_READY)