0

I have taken over code that has a lot of compound assignment operators in it. I think that compound operators are not 'really' MISRA compliant. I can't seem to find any reference to them.

I believe I understand what is happening and that it should actually be separated.

UINT16 A |= B; /* Incorrect */
UINT16 A = (UINT16)((UINT32)A | (UINT32)B); /* Correct */
UINT16 A <<= 1u; /* Incorrect */
UINT16 A = (UINT16)((UINT32)A << (UINT32)1u); /* Correct */

So, my questions are:

  1. Does MISRA frown upon compound assignments?
  2. Is there any kind of quick fix, instead of ignoring the warning?

Thank you,

Lundin
  • 195,001
  • 40
  • 254
  • 396
Christopher J. Holland
  • 61
  • 8
  • 1
    What warning are you getting? – interjay Aug 12 '20 at 10:00
  • 2
    What is variable definition `UINT16 A|=B;` expected to do? Take an indetermined value and set some additional bits? Are you sure it is about compound assignment operator or is it about using them in initializers? – Gerhardh Aug 12 '20 at 10:06
  • 1
    Lines 1 and 3 are syntax errors and lines 2 and 4 invoke undefined behavior. – chqrlie Aug 12 '20 at 10:43

1 Answers1

1

Does MISRA frown upon compound assignments?

Not as such. The rules for compound assignment are similar to the rules for simple assignment. MISRA speaks of assignment operators in general times, including all of them.

MISRA does however frown upon implicit type promotions, see Implicit type promotion rules. You can't understand these MISRA warnings unless you understand implict promotions.

Is there any kind of quick fix, instead of ignoring the warning?

You can't really fix this without understanding the warning. The only quick fix is to only use uint32_t everywhere and never signed or small integer types, but that isn't always feasible. Your original code would have been compliant if all variables were uint32_t.

In general, the latest MISRA only allows various conversions between types of the same essential type category. Unsigned integers is one such category, signed integers in another, and so on.

It isn't easy to tell how exactly your code violates MISRA without knowing the type of B and sizeof(int). This has nothing to do with compound assignment as such, except that compound assignment operators are a bit cumbersome to work with when it comes to implicit promotions.

MISRA frowns upon assigning the value of an expression (after implicit promotion) to a narrower essential type of the same category or to a different category. In plain English you shouldn't for example assign the result of a uint32_t operation to a uint16_t variable or a signed result to an unsigned variable. This is most often fixed with casting at appropriate places.

Regarding your specific examples, assuming B is uint16_t and the CPU is 32 bit, you get problems with implicit type promotion.

Since A |= B is equivalent to A | B, the usual arithmetic conversions will promote the operands to int in case of 32 bit CPU. So it is a signed 32 bit type.

Assume you had A << 31u - then this would actually have invoked an undefined behavior bug, which the rule seeks to protect against.

Sufficient fixes for MISRA-C compliance:

A = (uint16_t) (A | B); // compliant
A = (uint16_t) ((uint32_t)A << 1u) // compliant
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • Technically, both `uint16_t A = (uint16_t) (A | B);` and `uint16_t A = (uint16_t) ((uint32_t)A << 1u);` are UB. – chqrlie Aug 12 '20 at 10:39
  • Also is the `u` necessary in `A << 31u` ? – chqrlie Aug 12 '20 at 10:40
  • @chqrlie Why would they be UB? Conversion from unsigned to signed is well-defined if the value can be represented, which it evidentially always can when going from unsigned 16 to signed 32. Conversions from signed to unsigned types are always well-defined. See 6.3.1.3. The `u` is unnecessary in `A << 31u` as far as C goes, but I don't think MISRA-C is flexible enough to make a special case exception to the U suffix rule for the right operand of shift operators. – Lundin Aug 12 '20 at 10:56
  • You are missing my point: these lines define a variable `A` and use the value of the same variable in the initializer. If you meant to write `uint16_t C = (uint16_t) (A | B);` you would need further information about the types of `A` and `B` to achieve MISRA compliance, especially if `A` or `B` has type `char` I suppose. – chqrlie Aug 12 '20 at 11:16
  • The `u` suffix in `A << 31u` is useless and confusing. It gives a false impression that the operation is performed on unsigned `A`. If `A` is indeed unsigned and 32-bits, left shifting by 31 is no problem. – chqrlie Aug 12 '20 at 11:19
  • @chqrlie Ah yeah of course. Well, I don't think the source of the OP's problem is re-using the variable name in the initializer. As for the `31u` the sole purpose is to hush up static analysers without having to create a deviation from MISRA-C. – Lundin Aug 12 '20 at 11:21
  • Thank you all. Especially Lundin. It makes perfect sense. It was something I really already knew, but did not have any support to backup my claim. There are so many of these warnings and other warnings/errors that now I understand why they chose to just ignore the rule, which incidentally is "warning: conversion to 'u16' from 'int' may alter its value: [-Wconversion]" – Christopher J. Holland Aug 12 '20 at 13:27
  • @ChristopherJ.Holland `gcc -Wconversion` usually gives a lot of false positives, as do most static analysers. However, `A |= B;` and `A <<= 1u;` are actual MISRA-C violations and shouldn't be ignored, a bit shift on a signed operand could potentially be dangerous in some cases, as described in the answer. – Lundin Aug 12 '20 at 14:04