6
\$\begingroup\$

I am trying to upgrade an old firmware of a 32-bit MCU. The fw has many macros in it which uses various operations eg., logical shift etc. I want to replace these shift and logical operations. An example is the following macro.

#define ERROR_BIT(x)  ((x & ( 1 << 10 )) >> 10)

The main reason to get rid of these operations is to improve the code readability. What can be a good alternate approach here?

\$\endgroup\$
  • 9
    \$\begingroup\$ Replace with what? For me this is perfectly readable. Maybe just add comment that this isolate bit 10 from (I assume) integer. \$\endgroup\$ – Rokta Jun 1 at 7:16
  • \$\begingroup\$ Will it be possible to use some struct or union etc that can replace these operations? \$\endgroup\$ – homecloud Jun 1 at 7:30
  • 2
    \$\begingroup\$ Nope. Bitfields are compiler-specific, those shifts work everywhere the same. \$\endgroup\$ – Turbo J Jun 1 at 7:33
  • 3
    \$\begingroup\$ @homecloud You couldn't be using bit-fields, so don't study them. They are very poorly defined by the C standard, leading to brittle and completely non-portable code. Bit-shifts is the correct solution. \$\endgroup\$ – Lundin Jun 1 at 11:33
  • 2
    \$\begingroup\$ It might be slightly more readable as ( ( x >> 10 ) & 1 ), but it's fine either way. \$\endgroup\$ – Cristobol Polychronopolis Jun 1 at 19:32
22
\$\begingroup\$

The code you have posted is quite readable. C programmers are assumed to understand the meaning of commonly used operators and expressions. The bitwise operators in particular, in the context of embedded systems programming. They are however not required to understand the meaning of mysterious macros - I'll get back to that later.

First of all, there are other issues with the code not related to readability, namely that the shift operators are dangerous to use on signed types. For this reason, the 1 should always be written as 1u to guarantee an unsigned type. Also, there's a beginner-level bug: the macro parameter is not surrounded by parenthesis.

Furthermore, the right shift with 10 is strange and unnecessary. If the purpose of the macro is to give 1 or 0 depending on if a bit is set, you can convert it to a boolean expression by comparing the result with != 0, or by using !! in front of it. That way we don't risk bugs related to arithmetic shifts or implicit type promotions. Bugs fixed, we end up with this:

#define ERROR_BIT(x)  ( ((x) & (1u << 10 )) != 0 )

Did this improve readability? Not in the slightest! But it fixed 1 obvious and 2 potential bugs.

As it turns out, macros are not great at all for the purpose of increasing readability, quite the contrary. The first thing we should do to actually improve readability, is to replace the "magic numbers" with constants:

#define MASK (1u << 10)
...

#define ERROR_BIT(x)  ( ((x) & MASK)) != 0 )

where "MASK" should be given a more meaningful name depending on what the code does.

And this leads us to the root of the problem: namely programmers writing secret macro languages for the purpose of carrying out trivial operations. To vastly improve readability, we get rid of the macro:

if(var & MASK)

Which perfectly is readable to 100% of all C programmers. In a real-world example, you might have something like:

if(SPISR & SPISR_TXBUSY)
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Goal reached: Provoked an answer from the experts. :-) \$\endgroup\$ – the busybee Jun 1 at 11:35
  • 1
    \$\begingroup\$ @thebusybee I don't know why the answer from 'thebusybee' is deleted.. It was good to see that alternate solution also. \$\endgroup\$ – homecloud Jun 1 at 11:49
  • 1
    \$\begingroup\$ I really, really, really wish you would have advised the person to replace dangerous macros with type-safe inline functions... Although I totally agree with the remainder of the answer. \$\endgroup\$ – Cody Gray Jun 2 at 3:35
  • 1
    \$\begingroup\$ @CodyGray It's complicated. For basic stuff like using bitwise operations, you should not have macros or functions. For more complex operations, inline functions are overall better than macros, though they are not without problems of their own, such as namespace clutter and the inability to privately encapsulate the function (which you get with macros too). In many cases the correct solution is rather to use an ordinary function - and modern compilers may sometimes inline those. Yet another scenario is modern _Generic that can be used to increase type safety but requires macros to work. \$\endgroup\$ – Lundin Jun 2 at 6:29
  • 3
    \$\begingroup\$ Please, name that thing ERROR_MASK or ERROR_BIT_MASK instead of just MASK. \$\endgroup\$ – Guntram Blohm supports Monica Jun 2 at 7:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.