MISRA2012 Rule 12.1 Extra parentheses recommended

Question

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

Answer 1

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 the C language since it reserves those for the implementation (C17 7.1.3).

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.

Answer 2

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.)

Answer 3

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;
        case CHANNEL_2: return 1;
        case CHANNEL_3: return 2;
        case CHANNEL_4: return 3;
        case CHANNEL_5: return 4;
        default:        return 5;
    }
}

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)

Answer 4

When trying to fix some seriously odd code like this, it's often a good idea to take one or two big steps backwards.

We know that hPer refers to an array. We have some troublesome code that is indexing into that array and pulling out one of the channel states. But this code is, frankly, pretty awful. Even if the MISRA checker weren't complaining about it, any time you've got five nested ?: operators, performing a cumbersome by-hand emulation of what ought to be a simple array lookup, it's a sure sign that something isn't right, and that there's probably a better way to do it. So what might that better way be?

One way to approach that question is to ask, How is the ChannelState array filled in? And is there any other code that also fetches out of it?

You've only asked us about this one line that your MISRA checker is complaining about. That suggests that the code that fills in the ChannelState array, and any other code that fetches out of it, is not drawing complaints. Perhaps that other code accesses the ChannelState array in some different, hopefully better way. Perhaps the underlying problem is that the programmer who wrote this CHANNEL_STATE_GET macro was unaware of that other code, had not been properly educated on this program's coding conventions and available utility routines. Perhaps it's perfectly acceptable to directly index a ChannelState array using a channel value. Or perhaps there's already something like the map_channel_index function which I suggested in my other answer.

So, do yourself a favor: spend a few minutes seeking out some other code that accesses the ChannelState array. You might learn something very interesting.