Unpermitted operand to operator '++' [MISRA 2012 Rule 10.1, required]

Question

I am trying to fix the Misra warning for the modules written by others. I observed that ++ operation is being used on the enum.

I referred SE question which talks on the same topic. How do I resolve this error? Do I need to suggest the module owner, to change the implementation?

#include <stdio.h>

typedef enum
{  
    COMPARE = 0,
    INCONSISTENT = 10,
    WRITE,
    READ,
    FINISHED  
}TestsType;

static TestsType CurrentTest;

void fun1(void)
{

    if(READ != CurrentTest)
    {

            CurrentTest++;
    }
    else
    {
            CurrentTest = FINISHED;
    }
}

int main(void) {
    // your code goes here

    CurrentTest = COMPARE;
    fun1();
    printf("%d", CurrentTest);

    return 0;
}

I kept the enum like this in code purposefully to understand any impact. However, in actual code, it is as below.

typedef enum
{  
        COMPARE,
        INCONSISTENT,
        WRITE,
        READ,
        FINISHED  
}TestsType;

Answer 1

Incrementing an enum is just wrong!

enums were added to the language as a better alternative to #define for a number of constants, and were considered ints in other respects (i.e. a const array of ints). To enforce anything more would require run-time checking.

As enum values don't have to be contiguous, incrementing them makes no sense when they're treated as integers. If a compiler does allow it, it thinks it's incrementing an int, which can mean your value doesn't correspond to any value in the enum afterwards.

So my advice is "don't do it" even if a particular compiler lets you. Rewrite it to something explicit.

If you want to cycle through a particular range of states represented by contiguous integers, you CAN use an enum but only if you make its values contiguous too. Put lots of warnings about the definition explaining not to tinker. Then increment an int representing the state, which can then be compared to the enum safely.

Answer 2

The whole point of using a standard like MISRA is to avoid risky code. And there's no question but that incrementing enums is risky.

If you've got some code that increments enums, and it works well (under all conditions), it's only because of a number of interlocked assumptions and conventions which probably aren't all written down and which almost certainly won't be obvious to (and honored by) a later maintenance programmer.

So, indeed, there is no simple fix for this. Any simple fix (which might get your MISRA checker to shut up) will likely leave the inherent risks in the practice all intact -- that is, you might satisfy the letter of MISRA, but not the spirit (which is obviously backwards).

So yes, you should require (not just suggest) that the module owner change the implementation.

What might the revised implementation look like? I think it should have one or more of the following aspects:

1. Use an int and some #defined constants.
2. Have a separate, encapsulated function to map from one state to the next.
3. Use an explicit transition table to map one state to the next.
4. If there is a large number of states, and if most of them follow in sequence, such that a +1 increment would nicely encapsulate this (more cleanly and reliably than a bunch of arbitrary state transitions), go ahead and use a +1 increment, but with some accompanying assertions to ensure that the various assumptions hold. For example:

    enum state {
        OFF = 0,
        LOW = 3,
        MEDIUM,
        HIGH,
        EXCEPTIONAL = 10
    };

    /* States LOW..HIGH are assumed to be contiguous.  Make sure you keep them so! */
    /* If (and only if) you add or subtract states to the contiguous list, */
    /* make sure to also update N_CONTIGUOUS_STATES. */

    #define N_CONTIGUOUS_STATES 3

    enum state nextstate(enum state oldstate)
    {
        /* Normally performing arithmetic on enums is wrong. */
        /* We're doing so here in a careful, controlled, constrained way, */
        /* limited just to the values LOW..HIGH which we're calling "contiguous". */
        assert((int)LOW + N_CONTIGUOUS_STATES - 1 == (int)HIGH);
        if(oldstate >= LOW && oldstate < HIGH) {
            return (enum state)((int)oldstate + 1);
        } else {
            /* perform arbitrary mappings between other states */
        }
    }

The intent here is both to document what's going on, and ensure that if a later maintenance programmer changes the enum definition in any way that breaks the assumption that there are some consecutive states between which straight incrementation is allowed, the assertion will fail.

...But I hasten to add that this is not a complete solution. An even more important guarantee to preserve is that every state transition is handled, and this is even easier to violate if a later maintenance programmer adds new states but forgets to update the transition mappings. One good way to have the compiler help you guarantee this is to use a switch statement, although this then just about forces you to make every transition explicit (that is, not to use the +1 shortcut):

    enum state nextstate(enum state oldstate)
    {
        switch(oldstate) {
            case OFF: return ... ;
            case LOW: return MEDIUM;
            case MEDIUM: return HIGH;
            case HIGH: return ... ;
            case EXCEPTIONAL: return ... ;
        }
    }

The advantage of using a switch is that modern compilers will warn you if you leave an enum value out of a switch like this.