CODEWITHSUNDEEP
cembedded
m47h
m47h

Reputation: 1703

Cast to larger type in switch case

During work i saw the following code snippet and now i wonder, if there is a reason for casting the "case-values" to a larger datatype. I guess, that it is used to offer the possibility of having more than 256 different states later on, but then, the state variable must also be larger. Here is the code i am talking about:

#define STATE_1 (uint8_t)(0x00)
#define STATE_2 (uint8_t)(0x01)
...
#define STATE_n (uint8_t)(0x..)

void HandleState(uint8_t state)
{
  switch(state)
  { 
    case (uint16_t)STATE_1:
      // handle state 1
      break;

    case (uint16_t)STATE_2:
      // handle state 2
      break;

    ...
    case (uint16_t)STATE_n:
      // handle state n
      break;

    default:
      break;
  }

}

Is there another reason for this?

Upvotes: 3

Views: 4053

Answers (4)

Lundin
Lundin

Reputation: 213892

It looks like a slightly failed attempt to get rid of implicit integer promotions on a 8-bit or 16-bit microcontroller.

First of all, there is no way integer promotion can cause harm here, so preventing it is overly pedantic and just reduces readability. At a glance it looks like nonsense.

But.

Perhaps the coding standard used enforced that no implicit conversions are allowed? Which for example MISRA-C does. In that case, the code suddenly makes sense: they wished to surpress warnings from their static analysis tool.

Using explicit casts would then also be a way of demonstrating that you are actually aware of implicit type promotions (something that only a handful of C programmers are, sadly) and that you handle them in your code.

But if so, the programmer missed out one integer promotion taking place, namely here: switch(state). This is how switch statements work:

The integer promotions are performed on the controlling expression. The constant expression in each case label is converted to the promoted type of the controlling expression.

So if the programmer was concerned about implicit promotions, they should have written the code as switch((uint16_t)state) and then also kept the casts in each case.

If you sneered at the code in the question, consider the following:

  • Do you actually know what implicit promotions there are in a switch statement and did you consider what impact they might have on this code?
  • Do you know the meaning of the integer promotion rules? Did you know and consider that an uint8_t will get promoted to a (signed) int in this code?
  • Do you use a coding standard and static analysis tools at all, yourself?

Upvotes: 3

Lightness Races in Orbit
Lightness Races in Orbit

Reputation: 385194

Is there another reason for this?

No.

It's either a mistake, muppetry, or legacy (perhaps state and the macros used to be something else, but this particular code never got changed?).

I'm voting for muppetry, personally. Sometimes you run into code that somebody else wrote that is bad code, and that's just the way it is.

Upvotes: 6

too honest for this site
too honest for this site

Reputation: 12263

It might be intended to enforce unsigned int type promotion for the compares, but unit16_t is not guaranteed to be the same size either and just casting state would be sufficient.

Perhaps the state codes were enum labels in a previous version? These are by default int - but even then the casts make little sense imo.

Note that the casts even possibly suppress warnings about inappropriate size. Best would be to use an enum, or #define the labels just unsigned, e.g. 0x00U without fixing the type.

Upvotes: 1

Sam Estep
Sam Estep

Reputation: 13324

That just looks like bad style all around. You should have a single typedef for state:

typedef uint8_t State;

Then your code would look like this:

#define STATE_1 (State)(0x00)
#define STATE_2 (State)(0x01)
...
#define STATE_n (State)(0x..)

void HandleState(State state)
{
  switch(state)
  { 
    case (State)STATE_1:
      // handle state 1
      break;

    case (State)STATE_2:
      // handle state 2
      break;

    ...
    case (State)STATE_n:
      // handle state n
      break;

    default:
      break;
  }

}

Then if you later find you need uint16_t, you only need to change one line.

Upvotes: 1

Related Questions