embedded software boot camp

Don’t Follow These 5 Dangerous Coding Standard Rules

Tuesday, August 30th, 2011 by Michael Barr

Over the summer I happened across a brief blog post by another firmware developer in which he presented ten C coding rules for better embedded C code. I had an immediate strong negative reaction to half of his rules and later came to dislike a few more, so I’m going to describe what I don’t like about each. I’ll refer to this author as BadAdvice. I hope that if you have followed rules like the five below my comments will persuade you to move away from those toward a set of embedded C coding rules that keep bugs out. If you disagree, please start a constructive discussion in the comments.

Bad Rule #1: Do not divide; use right shift.

As worded, the above rule is way too broad. It’s not possible to always avoid C’s division operator. First of all, right shifting only works as a substitute for division when it is integer division and the denominator is a power of two (e.g., right shift by one bit to divide by 2, two bits to divide by 4, etc.). But I’ll give BadAdvice the benefit of the doubt and assume that he meant to say you should “use right shift as a substitute for division whenever possible”.

For his example, BadAdvice shows code to compute an average over 16 integer data samples, which are accumulated into a variable sum, during the first 16 iterations of a loop. On the 17th iteration, the average is computed by right shifting sum by 4 bits (i.e., dividing by 16). Perhaps the worst thing about this example code is how much it is tied a pair of #defines for the magic numbers 16 and 4. A simple but likely refactoring to average over 15 instead of 16 samples would break the entire example–you’d have to change from the right shift to a divide proper. It’s also easy to imagine someone changing AVG_COUNT from 16 to 15 without realizing about the shift; and if you didn’t change this, you’d get a bug in that the sum of 15 samples would still be right shifted by 4 bits.

Better Rule: Shift bits when you mean to shift bits and divide when you mean to divide.

There are many sources of bugs in software programs. The original programmer creates some bugs. Other bugs result from misunderstandings by those who later maintain, extend, port, and/or reuse the code. Thus coding rules should emphasize readability and portability most highly. The choice to deviate from a good coding rule in favor of efficiency should be taken only within a subset of the code. Unless there is a very specific function or construct that needs to be hand optimized, efficiency concerns should be left to the compiler.

Bad Rule #2: Use variable types in relation to the maximum value that variable may take.

BadAdvice gives the example of a variable named seconds, which holds integer values from 0 to 59. And he shows choosing char for the type over int. His stated goal is to reduce memory use.

In principle, I agree with the underlying practices of not always declaring variables int and choosing the type (and signedness) based on the maximum range of values. However, I think it essential that any practice like this be matched with a corresponding practice of always declaring specifically sized variables using C99’s portable fixed-width integer types.

It is impossible to understand the reasoning of the original programmer from unsigned char seconds;. Did he choose char because it is big enough or for some other reason? (Remember too that a plain char may be naturally signed or unsigned, depending on the compiler. Perhaps the original programmer even knows his compiler’s chars are default unsigned and omits that keyword.) The intent behind variables declared short and long is at least as difficult to decipher. A short integer may be 16-bits or 32-bits (or something else), depending on the compiler; a width the original programmer may have (or may not have) relied upon.

Better Rule: Whenever the width of an integer matters, use C99’s portable fixed-width integer types.

A variable declared uint16_t leaves no doubt about the original intent as it is very clearly meant to be a container for an unsigned integer value no wider than 16-bits. This type selection adds new and useful information to the source code and makes programs both more readable and more portable. Now that C99 has standardized the names of fixed-width integer types, declarations involving short and long should no longer be used. Even char should only be used for actual character (i.e., ASCII) data. (Of course, there may still be int variables around, where size does not matter, such as in loop counters.)

Bad Rule #3: Avoid >= and use <.

As worded above, I can’t say I understand this rule or its goal sufficiently, but to illustrate it BadAdvice gives the specific example of an if-else if wherein he recommends if (speed < 100) ... else if (speed > 99) instead of if (speed < 100) ... else if (speed >= 100). Say what? First of all, why not just use else for that specific scenario, as speed must be either below 100 or 100 or above.

Even if we assume we need to test for less than 100 first and then for greater than or equal to 100 second, why would anyone in their right mind prefer to use greater than 99? That would be confusing to any reader of the code. To me it reads like a bug and I need to keep going back over it to find the logical problem with the apparently mismatched range checks. Additionally, I believe that BadAdvice’s terse rationale that “Benefits: Lesser Code” is simply untrue. Any half decent compiler should be able to optimize either comparison as needed for the underlying processor.

Better Rule: Use whatever comparison operator is easiest to read in a given situation.

One of the very best things any embedded programmer can do is to make their code as readable as possible to as broad an audience as possible. That way another programmer who needs to modify your code, a peer doing code review to help you find bugs, or even you years later, will find the code hard to misinterpret.

Bad Rule #4: Avoid variable initialization while defining.

BadAdvice says that following the above rule will make initialization faster. He gives the example of unsigned char MyVariable = 100; (not preferred) vs:


#define INITIAL_VALUE 100
unsigned char MyVariable;
// Before entering forever loop in main
MyVariable = INITIAL_VALUE

Though it’s unclear from the above, let’s assume that MyVariable is a local stack variable. (It could also be global, the way his pseudo code is written.) I don’t think there should be a (portably) noticeable efficiency gain from switching to the latter. And I do think that following this rule creates an opening to forget to do the initialization or to unintentionally place the initialization code within a conditional clause.

Better Rule: Initialize every variable as soon as you know the initial value.

I’d much rather see every variable initialized on creation with perhaps the creation of the variable postponed as long as possible. If you’re using a C99 or C++ compiler, you can declare a variable anywhere within the body of a function.

Bad Rule #5: Use #defines for constant numbers.

The example given for this rule is of defining three constant values, including #define ON 1 and #define OFF 0. The rationale is “Increased convenience of changing values in a single place for the whole file. Provides structure to the code.” And I agree that using named constants instead of magic numbers elsewhere in the code is a valuable practice. However, I think there is an even better way to go about this.

Better Rule: Declare constants using const or enum.

C’s const keyword can be used to declare a variable of any type as unable to be changed at run-time. This is a preferable way of declaring constants, as they are in this way given a type that can be used to make comparisons properly and enabling them to be type-checked by the compiler if they are passed as parameters to function calls. Enumeration sets may be used instead for integer constants that come in groups, such as enum { OFF = 0, ON };.

Final Thoughts

There are two scary things about these and a few of the other rules on BadAdvice’s blog. First, is that they are out there on the Internet to be found with a search for embedded C coding rules. Second, is that BadAdvice’s bio says he works on medical device design. I’m not sure which is worse. But I do hope the above reasoning and proposed better rules gets you thinking about how to develop more reliable embedded software with fewer bugs.

Tags: , , , , ,

83 Responses to “Don’t Follow These 5 Dangerous Coding Standard Rules”

  1. Hello,

    Very nice post, dealing with many bad ideas or optimization myths. I would add some points:

    @Bad Rule 1:
    It is really easy for compiler writers to detect such “power of two” divisions and replace them with shift, so many compilers will generate exactly same object code even with O1.

    @Bad Rule 2:
    I would personally add: If stdint.h is not available use #ifdefs to typedef standard types to C99′s portable fixed-width integer types. It is more convenient not to interface with code using millions variations of fixed-width integers (like u8, uint8, etc).

    Also using fast types may be vary useful when declaring local variables.

    @Bad Rule 5:
    Using #defines has one advantage – if place of #define is far away from place of use (for example #defines based configuration) you can use #ifdef and #error directive to check some assertions – for example that some buffer has size being power of two etc.

    • Eric Wertz says:

      I agree with you about your specified virtue of using the preprocessor to help mitigate some of the liabilities of simply using a #define. I’d like to add two more.

      First, #defines don’t consume (precious, if you’re on such a platform) SRAM where as “const ” constructions generally do. If one is documenting a ton of symbols, in particular in a header file for general use, then the probability exists that the majority of the defined symbols will be unused, and will consume SRAM. If the tool chain is smart enough (or if you’re willing to pay a premium for one that does), it could eliminate the unused constants, but many/most don’t. Enums don’t have this liability, as they’re merely type definitions and not variable(-like) declarations.

      Second, the problem with allocating space for consts is that it (in practice) precludes the compiler from encoding the constant as an immediate in the operand for the instruction if it’s small enough, which also benefits code space as well as performance. Most compilers _are_ smart enough to do constant folding so that even if the constant has to go into SRAM, only one instance will be created (albeit perhaps only in module-scope) depending on the toolchain.

      For uses of consts as constant symbols in module scope that are declared and not used, the first reservation is a non-issue and isn’t the worst practice. In my opinion, enums are fine solution iff you fully intent to make full use of the type definition that comes out of it, and don’t simply use it as a symbolic constant that gets slopped-around between variables of other types (chars, uintN_t, etc.).

      I haven’t checked but my memory of enums was that they’re “int”-sized, which has all of the space-penalty liabilities for those of us that work on resource-constrained systems, if my target requires only 8/16 bits.

      About your reservations regarding BadRule2, why not just use typedefs rather than #defines to establish the C99 types? By not using typedefs you don’t get any of the strong-typing benefits that Michael rightly wants to perpetuate, whereas #defining them merely maps them to their weakly-specified types (int, long, etc.) without almost none of the benefits.

      The other comment is that most compilers are also smart enough to be able to identify the many of the worst #defined constructions at their point of (mis-)use — in particular, constants known to be assigned inappropriate lvalues (signed where unsigned is required, or stores into datatypes are too small for the #defined rvalue). Maybe not in all cases, but in many. I’d love to know about common counter-examples, although only “reasonable”, non-toy compilers need apply…

      As far as BadRule1 and BadRule3 are concerned, I can’t help but say “The 1980’s called and they want their compilers back…”. Unless one can definitively otherwise, the compiler *is* smarter than you are. These two “rules” are clearly dead-fish waving.

      • Jason says:

        With most compilers, using an enum will take up no space on the target, will be optimized just as well as if you had used a #define, and has the added benefit of better debug information.

        I don’t know where BadArticle got their info, but I learned C in the 80s with Thomas Plum’s excellent “Learning to Program in C” and even back then it recommended enum over #define specifically for better debug information.

      • Tim Wescott says:

        While “const XXX;” will cause the compiler to allocate space for XXX (possibly in ROM, depending on your tool chain),
        “static const XXX;” will often cause the compiler to optimize out the allocation — certainly the gnu compiler is pretty good about this for “simple” data types, in my experience.

        As with all compiler optimization comments, however, your milage may vary — I always check the emitted assembly code if it’s important to know.

        • Tim Wescott says:

          Unless you take the address of the static constant thing you’ve defined — then the compiler is forced to shove it into memory.

          • Harlan Rosenthal says:

            Well, yes, but if you take the address of a #define, then the compiler would ALSO be forced to create a memory constant. But if you’re taking the address of something, most of the time you’re expecting that address to be a place to STORE things (like a return value from a function), so taking the address of a constant sounds like something is confused in the first place.

        • Lukasz says:

          I agree – static const xxx defined in the ‘header’ file(s) will be optimised out, and are safest (provide control and some control of the type + checking for that type etc).

          Enums might be a good idea, but note that various compilers might turn them into different types (e.g. RVCT uses smallest possible type that can handle enum range – so there can be surprises if enum is extended one day with a value big enough to change the actual sizeof(enum_type). This, however, probably matters more if you’re actually using this ‘enum’ as type ( e.g. use it in structs, as function parameters etc.).

          Another bad-thing about bit-shifting comes often from people being not really aware of the signed/unsigned type of the shifted value. And this could lead to somehow hard-to-find bugs, as for signed – shift can be ‘sign-extending’ or ‘zero-extending’ depending on the value.

          • Keith Thompson says:

            The size of an enum *type* can vary from one compiler to another.

            But an enum *constant*, oddly enough, is always of type int. (C++ has different rules.)

            For example:

            enum e { foo, bar };
            enum e obj;

            “obj” is of type “enum e”, and its size can vary from one compiler to another (though on some systems it may be specified by the ABI).

            But “foo” and “bar” are constants of type int, with values 0 and 1 respectively.

            This makes an enum declaration with no tag a convenient (but hackish) way to define constants of type int:

            enum { foo = 42 };

            A “const” declaration is more flexible in that you can use any type you like, but the resulting object is not a *constant*; for example, it can’t be used in a case label:

            const int foo = 42;
            switch (blah) {
            case foo: /* compile-time error */
            }

      • David Warman says:

        enums are given the smallest storage type that can represent the range. So the example for OFF and ON will compile to a byte type. This led to some very tough bugs trying to debug structs with enum fields shared between C host and ASM in co-processors too small for C because (a much younger) I assumed the int size mentioned. I _really_ do not like the cognitive dissonance of this language design decision. On the one hand, I would like to be able to control the representation separately fron the range (with compiler errors if I go too small), but using enums vs #defines give the debugger information to show values symbolically, itself often invaluable. Especially when handling communication protocols.

        int itself is a bother, since it is defined to be the size of the natural integer represention of the underlying CPU registers. So it could be anywhere from 8 bits to 64 bits (on to 128 …). For constant products not an issue, but I have had to shepherd a program’s migration from Windows 3.10 through Win 32 and on to 64 bit processors.

        Finally, and this is particularly relevant to the embedded world, C compilers rarely if ever allow for extensions that take advantage of the custom hardware typically present – extra registers, regiters with side-effects, accelerator instructions, et. For those one has to write ASM functions and call them (or inline them) explicitly. So representation sizes significantly impact C argument passing and result return rules. I have actually encuntered one C compiler that optimized those rules away – breaking the register allocations – without ever looking at the ASM code (it couldn’t), resulting in very bizarre runtime behavior. Showing dis-assembly in the debugger revealed the problem, and fortunately they had implemented pragmas for forcing argumant passing layout, but! Left me temporarily speechless that they had done that.

  2. Carle Henson says:

    I think whoever wrote rule 1 must be living in the far distant past when compilers were not intelligent in terms of optimization and we had to wring every last cycle out of the code. Perhaps there are still severely limited processors with outdated compilers but all of the compilers I have used in many years would do the shift optimization for you if it can.

  3. Eric Smith says:

    I think an even better rule than always using the C standard’s fixed-width integer types is to use the standard’s minimum-width integer types where the memory usage isn’t critical. In other words, for a value from 0 to 10000 that is going to be in a data structure that has a lot of instances, I’d use uint16_t, but where the size is less important, I’d use uint_least16_t. This gives the compiler the flexibility to use a wider integer if it wants to, possibly for optimization. For example, on some 32-bit processors, using uint16_t might force the compiler to add masking instructions, while it might be more efficient to use 32-bit storage with no masking, and uint_least16_t allows that.

    For code where maximum performance is essential, perhaps in an inner loop, I’d go one step further and use uint_fast16_t, which tells the compiler both that the width must be at least 16, and that I want whatever type will make the code fastest.

    • Thomas Thorsen says:

      Actually you should use uint_fastXX_t to avoid masking instructions. For example, on a 32bit ARM processor, uint_least16_t will be a 16bit type and subject to masking instructions following arithmetic operations on the register that contains the value.

      I think it is fair to say that the uint_leastXX_t types are quite useless on any platform that provides the full set of uintXX_t types, as they are required by the standard to be identical types in this situation. Note that the uintXX_t types are not mandatory in C99, but i doubt anyone would consider not providing them!

      • David Brown says:

        There are some processors (particularly DSP’s) that can’t access 8-bit, or perhaps even 16-bit data. On such targets, a “char” is 16-bit or 32-bit, which is still valid for C. Then there is no uint8_t and perhaps no uint16_t either – but uint_least8_t is still there. So if you want to write code that will work on such a DSP and will use the smallest type possible for better code on other cpus, you use uint_least8_t, etc.

  4. Hans Van Ingelgom says:

    Another bad idea in my opinion is using enums for bit fields, like typedef enum { blinking=1, bold = 2, inverted = 4 } fonttype; If you define an enumerated type, you are saying there is a type of which the values can be enumerated (duh!). If you then say fonttype t = bold | blinking, the value of t is not a member of fonttype. I’ve seen quite a lot of these constructs, however I always think it’s wrong. Or is it just me? Also if I remember correctly, C++ makes a much bigger fuzz about this.

    • scott says:

      i actually don’t have a problem with this, at least if there is sufficient documentation near the definition of fonttype that indicates that the enum is a bit field. although when i read the assigment fonttype t = bold | blinking; i can’t help but to think that t is bold *or* blinking instead of bold *and* blinking.

      kinda wish there was a key word that enforced it like the [Flags] attribute in c#.

      • Eric Wertz says:

        I can’t condone this. If you believe at all in what the abstraction of what an enum is “supposed” to be, using an enum in a context where bitwise operations are desired, is an abomination. In the purest sense, one shouldn’t even “know” what the _range_ of values are in the enumeration. However, we all know that being able to assign them concrete values has so much value in the real world that the language has conceded to break the most-pure abstraction of what an enumeration is.

        Bitwise operations are intended for uints, or bitfields if you really the syntactic sugar in your coding diet.

        If you don’t trust yourself to use “|” and “&” correctly, then you should be sticking to bitfields and stick with the logical operators. This just isn’t the right time or place for an enumerated type. If you’re really committed to writing bug-free code, one needs to recognize constructions that both the reader (and writer!) of the code get hung up on, and steer clear of them.

    • Ashleigh says:

      Agreed – enums should not be used for bitfields. Although you can have enums where the enumerants have any value you care to assign (just a named number) I find it more helpful to think of then as a whole new type which is not compatible with addition or logical operators.

      So things like bitfields need to use either a struct with bit field parts in, or #defines.

      “Bad rule #5” should be considered alonside the compiler. If the compiler generates bigger code or larger constant sections when declaring “const” then there may be good reason to use #define. In general though, the advice to use const is better – the compiler does more work.

    • Tim Wescott says:

      To add to the comments against this: at one point, Code Composer for the TMS320F2812 processor would assume that any comparison to an enum type that was not expelicitly defined was de-facto false. So in Hans’ case,

      if (mode == (blinking | bold))
      {
      etc.
      }

      would get optimized out.

      To my horror, about a decade ago, I managed to write several thousand lines of parsing that got optimized out to exactly nothing. Worse, the project was in a time crunch so it was left to someone else to replace all the ‘enum’ stuff with ‘static const unsigned int’.

  5. As far as rule 5 goes, I’ve been bitten by using const int instead of a define.

    #define SLOTS 10
    int my_array[SLOTS]; /* Standard C array */

    const int slots = 10;
    int my_array[slots]; /* Technically this is a “Variable” Length Array */

    • Gauthier says:

      +1, and the following cannot be done with const:

      #define TICK_FREQUENCY (50U)
      #define SECONDS_IN_PERIOD (60U)
      #define TICKS_IN_PERIOD (TICK_FREQUENCY * SECONDS_IN_PERIOD)

      Furthermore if the const/define value is to be used in several modules, where is the const supposed to be defined? In my opinion it must be in the c file, and declared as extern in the h file. This forces you to touch the c file if the variable needs changing. Const takes also memory (although little), which define does not.

      On the other hand, const are easier to debug.

      • Scott says:

        You can declare it static const and put it in the header. It will function like a define (assembler uses literals) with the benefit of types.

  6. David says:

    “how to develop more reliable embedded software with fewer bugs.”?
    Allow peers to review it: http://youtu.be/nFZGpES-St8?t=5m

  7. Ark says:

    Re Bad Rule #5:
    It is not bad by itself (floats can’t be enums); it looks like a restatement a MISRA rule deprecating “magic numbers”
    The point is that symbolic names should be used.

    C’s const objects cannot be eliminated from storage so they are not the same as parameter-like macros.

    A case for #define in favor of enum “is brought to you by” sizeof and, correspondingly, offsetof. size of an enumerated type is a cross-platform mess.

    • Phil King says:

      There is no MISRA rule deprecating “magic number” – MISRA does not (generally) include guidance related to style.

  8. Ark says:

    Re Bad Rule #4
    Sensibly used, it is IMO a great rule. Consider a “not assigned before use” bug: I just forgot to assign a value in one of the conditional paths. Static analysis tools (e.g. Lint) will spot this rather reliably. However, early initialization to a bogus “safe value” defeats this detection at build time.

    • Venkata Viraraghavan says:

      I think this rule is to do with globals. In fact, I’d say “Initialize global variables to known INvalid values”. That way, the issues with soft reset will show up in the first run of the code rather than in the second. If initialized to valid values at definition and not in an init function, they would hold previous (undefined) values during a soft reset. This could eat up debugging time after the first run has deemed that functionality is alright.

      • Tom the Toolman says:

        Unnecessary initialization costs code space. On some platforms, this is critical, so we have to byte the bullet and just be careful.

  9. Eduardo says:

    In the beginning of my education and then the first years of my professional career I used to believe many of these rules and their arguments as golden rules. Mostly because I was taught that way. Many of my college professors felt pretty important when they gave us examples that proved these rules (like the = rule) by compiling both examples and showing us the results, having predicted them before compile time (as a student, one thinks compilers are magical). My point is that these professors grew up (professionally) programming in assembly, and we have to remember that the first compilers weren’t as good as the ones we have today, it’s easy to understand why many people think some of these rules are important.

    I still defend the importance of knowing the processor architecture, but it’s also important for code to be readable. This is something I learned later on, when working with more people, when my code was peer-reviewed or was worked on by other people, readability is king! Optimize where you really need to optimize, elsewhere, make the code easy to read.

  10. Jeremy says:

    Enums for bitfields are more platform and compiler independent though if you’re looking at port pins on a processor. The only reason to use an enum is one of many methods to name a port IO to a particular variable.

    It’s not my method of choice, I would much rather have a constant structure array containing a port pointer and the port I/O pin, and then use the enums to index into that array, but that’s just me.

    What I hate seeing though is people using enums to specify bits in I/O registers. There are just so many ways this can shoot yourself in the foot, such as changing compilers, changing from 8-bit to 16-bit or 32-bit processors, etc. It can be even worse if you’re changing processor manufacturers!

  11. Trenton says:

    Never shift when you mean to divide. If you really are convinced that you are better at optimizing than your compiler use a macro like divide_x_by_32(x) and hide your shifting inside there. Also, shifting doesn’t work for products or quotients if the number being shifted is negative. Not all CPUs have single cycle shifts, some have single cycle divides, etc. So using a macro makes it easier to port.

    One reason to choose not to initialize variable when declared is to alter the time when initialization takes place. Initialized variables are usually initialized at reset, before main(). Uninitialized variables assigned later in the code are usually initialized when the core executes. This amortizes the time to initialize variables.

    Using const variables instead of #defines occupies storage, which may or may not be what you want. You need to be careful when using enums since many compiler treat then as signed integers, which may not be what you wanted. For example, passing narro enums as wide arguments may cause the compiler to add code to sign extend them.

    • david collier says:

      um, doesn’t a #define specify a sequence of characters. so compilers don’t treat them as signed anything, they put the characters in the matching place, and they get treated as if they belong there. Shouldn’t you be able to get the desired effect by specifying the literal value fully?

      • Trenton says:

        First, what I wrote about variable initialization is incorrect with respect to automatic variables in a function invocation.

        Second, what I meant about signed numbers in shifts is that -4 >> 1 doesn’t equal -2.

        If you use a #define to create a constant, say @define myvar -5, and then you pass that as a parameter to a function, say f(uint32_t x) in a call like f(myvar) then the preprocessor replaces f(myvar) by f(-5) before the compiler sees it. In this case you may get what you wanted.

        If you make an enum { one = 1, two, three} then usually that will be treated as a signed value of some fixed size (perhaps int) or a signed value of the smallest size big enough to represent the value; how this is done is compiler dependent and may or may not be under programmer control. Assuming that the enum becomes an int, if you then call f(long x) as f(one) then the compiler will convert the enum-as-int to a long. The point was that passing enumerated values as parameters may cause the compiler to inject code to massage them on the way into the function.

        • david collier says:

          as discessed elsewhere on thsi page

          “Second, what I meant about signed numbers in shifts is that -4 >> 1 doesn’t equal -2. ”

          is incorrect. It may, or it may not, it’s implementation dependent.

  12. Bill Davy says:

    Rule 3 may also have the undesirable tendency to introduce another constant (99 in your example) requirng either an expression using a constant (MaximumValue – 1) ora new cosnat to be (‘define’d ?) MaximumLessOne.

    Rule 4. One reason for using #define is when a compiler cannot use conts variables to work out an array size. best to avoid such puerile compilers but sometimes one has to. Also, for a bit field widths, a literal seems to be required. But far better as a rule, “have no literal constants in your code – use const (best) or #define (if required).”

  13. Tomasz says:

    @Bad Rule #5

    IMHO declaring a constant as #define has its advantages in many cases, e.g. when one can’t afford to waste too much RAM. One other reason – in small and slow environments, i.e. 8 bit uCs, it makes sense to load an immediate value into a variable, since it’s faster than loading one const to a register and only then copying it to another variable. There are times when I have to do such optimization due to sb-else’s poor hardware design 😉 (e.g. a system that was meant to do sth very simple now has to do more complex tasks on the same machine), although I agree it needs extra care from my side. This is a particular situation, because I always know that the code is not going to be maintained by anybody else.

    • David Brown says:

      If you declare something as “const int two = 2;” at file scope, it has external linkage. That means the compiler has to assume that anyone can change its value (yes, “const” variables are variables – not true constants) from other modules that the compiler cannot see at compile time. So the “constant” “two” gets allocated space in ram (or sometimes flash), and this space must be read when you use it. And “x = x / two;” means using a division instruction or library call.

      The correct way to define such constants is “static const two = 2;” – then the compiler knows exactly when and how “two” is used, and can see that it is never changed. It can then substitute the literal value 2 for “two” throughout the code, and “x = x / two;” becomes a right-shift operation.

  14. Konrad says:

    Re Bad Rule 1:
    The important thing to remember about using right shifts for division is that it rounds the quotient of a signed dividend towards minus infinity. -1 >> 1 is -1, not 0.

    I suspect (as the original code is not shown) that the problem of mismatched loop size and shift width is easily remedied by preventing the code from compiling when these values do not match. In this case, rather than testing for !(loop_counter % 16), it would be better to define log2_size as 4, use !(1 <> log2_size when calculating the average.

    The critique that the resulting code can only be used for power-of-two sample sizes (and not for a sample size of say, 15) is offset by the fact that the code runs much quicker for these sizes, as both the division and the detection of that the loop counter is 0 modulo 16 (or whatever). Hence, run-time is quite non-linear in the choice of sample size, and if the underlying problem is robust in the sample size (quite likely, since we are calculating averages), it makes sense to program run-time performance rather than for unnecessary generality.

    In fact, coding in this way prevents subsequent maintainers from lightheartedly changing the value such that this particular section of code runs drastically slower.

    In general, integer arithmetic has many interesting properties that one can exploit; compilers cannot do this in general. More importantly, it has limitations (such as overflow) which prevent it from being applied blindly. Mastering this requires practice. A piece of code using shifts rather than blindly applying division gives the reader at least some reassurance that the original coder has given some thought to these matters.

    A programmer with a limited vocabulary (i.e., is conversant with only a small number of idioms) is a programmer with a limited vocabulary. How low shall we go in the sake of “readability”, that is, limiting our use of idioms?

    • david collier says:

      eh? At the instruction set level there are at least 3 types of right shift
      1 duplicate the new top bit from the old one
      2 carry in zero
      3 carry in something external

      3 is unlikely to be generated by the compiler,

      Does the C standard specify which of 1 or 2 is meant by >>

      • Trenton says:

        @Does the C standard specify which of 1 or 2 is meant by >>

        WG14/N1256 ISO/IEC 9899:TC3

        6.5 Expressions
        4 “Some operators (the unary operator ~, and the binary operators <>, &, ^, and |, collectively described as bitwise operators) are required to have operands that have integer type. These operators yield values that depend on the internal representations of integers, and have implementation-defined and undefined aspects for signed types.”

        6.5.7 Bitwise Shift Operators
        5 “The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a nonnegative value, the value of the result is the integral part of the quotient of E1 / 2E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.”

        • david collier says:

          thanks.

          To summarise then – either 1. or 2. will meet the specification, so C does NOT sepcify which of the two will be used in some particular case.

          which means Konrad was not correct with “-1 >> 1 is -1, not 0.” — in fact the result is implementation dependent.

  15. Bruno Richard says:

    About rule #5: I do not fully agree. Compilers will not optimize the constant variables away, and on small microcontrollers this has an impact on the RAM or Flash consumption.

    For example:

    const unsigned short val = 10; // Will eat up 2 bytes of RAM on some compilers

    You have to check what your specific compiler does.

    My advice on this point is exactly the opposite: Use const variables on your large (Widows/Linux/OSX) projects, but stick to #defines for constants on microcontroller projects!

    • (From GCC experience)
      You should declare this variable also static (so it can be optimized out by compiler), or enable -data-sections for compiler and -gc-sections for linker, so linker will remove it.

      If compiler is the only part that does optimization (as GCC before -flto), you must be careful when defining global scope function/variables.

  16. David Brown says:

    I agree with most of this post, with a few comments.

    I don’t know where Mr. BadAdvice’s rules come from – perhaps they are aimed specifically for a small and limited cpu (my guess is an 8051) with a limited quality compiler. But if you have to follow his advice to get fast code, you should invest in better tools (and better cpus). Oh, and don’t forget to enable optimisations on the compiler (it’s amazing how many people /do/ forget that!).

    Bad rule #1 – There are two more reasons to prefer divides to shifts. When you are dealing with signed data, it is no longer easy to replace multiplication or division with shifts. It gets messy – it is better to write the arithmetic operation you actually want, and let the compiler sort out the details. Secondly, the compiler may be smarter about optimising multiplication and division by constants than you are, especially for non powers-of-two. So I agree with you here – write the source code the way it should be written, and let the compiler optimise the implementation.

    Bad rule #2 – Again, I agree with your principle. is there for a reason, and its types should be used. int_fast8_t, uint_least16_t, etc., are also useful, especially in portable code. I’d go a little further than you, however, in that the width of an integer /always/ matters, so the great majority of types used in a program should be from stdint.h. I also hate the use of “char” when people are talking about a number – a “char” is a “character”, or letter in a string. “unsigned char” and “signed char” are totally meaningless concepts – use “uint8_t” or “int8_t” as they are for numbers. I also hate people who use “unsigned” or “long” as a type – these are adjectives (and type qualifiers), not type names. Programs should make sense when you read them – just because the original C authors had crappy keyboards and had to take shortcuts, does not mean modern programmers should follow their bad habits.

    Bad rule #3 – What is there to say, but that I agree? I suppose Mr. BadAdvice was using a small target cpu and a limited compiler that generated inefficient code for >= and <=.

    Bad rule #4 – Again, I think BadAdvice must have a poor compiler – any decent compiler will delay the initialisation until it is useful, if it makes the code smaller or faster. But again I'd go further – initialise every variable as soon as you know its initial value, but don't declare any variable until close to when you need it, and within the smallest practical scope. C99 has been around for a /long/ time – the days of piles of variable declarations at the start of a function are thankfully gone.

    Bad rule #5 – This is the only one I don't agree with entirely. Declare constants using "enum" when it makes logical sense, with "static const" when possible, and with #define when necessary. There are some cases when you can't get away with "static const" constants, such as array declarations. But always make sure you use "static const" rather than plain "const" – it tells the compiler that it knows every use of the constant, and can therefore optimise freely with it as though it was a #define'd value. Without "static", the compiler must put the constant in memory and assume that some other module will change its value unexpectedly. (Remember, C and C++ are different in this point – in C++, "const" is effectively "static const" by default.)

  17. tomkawal says:

    I would not argue when to use const,enum or define – look always at the data representation, know your priorities.
    But ‘rulez’ which have no mean are ridiculous. I laugh and laugh at rule #3.
    But there are issues with the hardware, the coders are rarely aware of:
    Examples:
    Naive coding:
    If (++seconds == 60) seconds = 0;
    Electronics designer code:
    If (++seconds >= 60) seconds = 0;

    I would like to post the recommendation: “avoid ‘==’, if you ca use ‘>=’ or ‘<='
    Otherwise any spurious memory corruption will cause disaster.
    I've seen the clock stuck at 65 seconds, as an example.
    The first line of defence is good HW design for EMC, but second: proper plausibility checks in SW.

    regards
    Tomasz

  18. david collier says:

    Rule 1 is not a ruile. The rule should be

    “be aware that understanding the relationship between shifts and divisions ( or multiplications ) by 2 can sometimes give you more efficient code.”

    I have a lovely block of code for an MSP430, which has a 12-bit AtoD. I DMA a block of AtoD values onto a buffer, and then “block add” them to another buffer.

    After doing this 16 times, the results are averaged 16-bit signed numbers I can use elsewhere.

    If written without adequate comments it’s horrific. With the right comments it’s a thing of beauty.

  19. Lundin says:

    #1 is not really a programming rule, it is an optimization advise. There are actually still dumb-enough compilers around, that will not do this shift optimization for you. So knowing about it is good, although it should only be used when one has verified that 1) The compiler is indeed dumb and 2) that the division is an actual performance problem and not just a non-existent problem conjured by the programmer.
    I wouldn’t call #1 bad nor good, it is just a commonly used optimizer trick.

    #2 is generally a good rule. The rule doesn’t say explicitly that the native primitive data types of C (int, char etc) should be used. I think everyone agrees that either using stdint.h or typedef:ed integer types is a must, using the native primitive types is a very bad idea for many reasons.
    But the rule rather says: pick a size of the variable according to the need, which is another matter entirely. This is especially important on small embedded systems (8- and 16-bit MCUs). On the other hand, on 32-bit or larger CPUs, using anything but 32-bit integers is most of the time a bad idea, because as soon as you do you expose yourself to the whole ballet of implicit promotion rules: one of the most dangerous “features” of the C language.

    #3 is just strange. Why would you do that for? A common cause for bugs are programmers that assume that the logical opposite of “less than” is “greater than”. While in reality, the opposite is “greater than or equal”. Anyone with a software and/or electronics degree has likely been sufficiently exposed to boolean algebra and discreet mathematics to understand this.

    #4 is a very good rule for embedded systems! The main reason is that the C standard states that all variables that are global or static must be initialized before use. Either to the value the programmer initialized them to, or if they didn’t, to the value zero. This initialization must be done before the program is started, it is required by the C standard. For an embedded system where code resides in flash rather than RAM, this means that a “copy-down” snippet must be executed in runtime before main() is called, which copies down values to all statics/globals.
    This “copy-down” is very bad for several reasons. First of all, it means that as soon as your CPU hits reset, there will be a startup delay while the copy-down is executed. This causes your program to start up much slower. Which in turn could be disastrous if the program has some important code at top of main() that must be executed shortly after reset: watchdog setup, low-voltage detect, clock initializations, pull resistor or I/O port enables, mode registers etc etc.
    But the copy-down is also bad because it may very well take ages between the copy-down and the time when the code using the variable is executed. During this time the value sits in RAM, and good practice is to never rely on RAM. High integrity embedded systems typically forbids such practice, and also requires that all values in RAM are updated from flash on regular basis.
    To avoid all mentioned problems, it is common practice to write code that does not rely on static/global initialization, ie the code does not rely on initialization at all. Most embedded compilers also have a non-standard option to disable initialization at startup. If you wrote the code so that it does not rely on initialization, it will be portable even if this -very- common non-standard extension is present in the compiler.
    Ok so what if the variable is placed on the stack (auto) rather than static/global? The programmer must make sure to set it at some point before using it. If they forget it… well, it is no biggie. Since all professional programmers use static analysers (right?), that will instantly find such simple bugs. Real programming rule standards like MISRA-C explicitly states that all -automatic- variables should be initialized.
    To sum this up, #4 is a very good rule for embedded systems. For RAM-based PC fluff however, it doesn’t really make any sense.

    #5 is just personal preference, there is no real advantage/disadvantage to it. It isn’t really worth debating.
    To use enum to get -better- type safety is however a very bad idea! It is (signed) int by default, and thus it is as bad as using the standard “int” type. And then comes all the dangers of implicit promotions again.
    const and #defines can be declared unsigned, but enums can’t.

    • David Brown says:

      Regarding #2, you misunderstood Michael’s point. It is correct that you should often use a type that is no bigger than you need, especially on small cpus (though be aware that on bigger cpus, code that accesses smaller types is often slower). The point was that you should use “uint8_t” rather than “unsigned char”, etc.

      Your comments on #4 show a misunderstanding of some basic C concepts. First, consider statically allocated data (file scope, or function static data). Following the C standard, they must be initialised before main() starts – either to their specified initial value, or to 0. So if you omit the initialisation value, thinking to set it within the main code, then the C startup library code will write 0 to the data. In other words, you won’t save much time at the pre-main() stage, since the cpu will have to write 0 to the uninitialised data instead of copying in the real initial value. And it is generally much more efficient (in time and space) to let the pre-main() routines initialise the data rather than doing it explicitly in the main code.

      If you have actions that must occur as soon as possible after reset, and can’t wait for data initialisation, then most embedded toolchains have hooks for adding your own pre-main() code. And anyway, you’ve probably got the hardware wrong – after all, reset and oscillator/pll startup typically takes longer than the pre-main() data initialisation on most processors. So if you think you need something happening that fast after reset, the chances are high that you’ll be getting startup glitches anyway.

      For auto variables (on the stack, or in registers), C provides no automatic initialisation – you need to do it every time. If the programmer forgets it, then it /is/ a “biggie” – the programmer has written incorrect code, and the program won’t do what it is meant to do. Most professionals use some sort of static analysis (or at least a compiler with good warnings) – but very few work by writing sloppy code and expecting the static analysis to spot the bugs.

      For #5, you are correct that C compilers treat an enum as an int (or unsigned int, long int, or unsigned long int if necessary depending on the value of the enum constant) – though some may be configured to use smaller types (in the manner of C++). However, you may like to remember that a number literal (such as you get from “#define two 2”) is also considered an int (or unsigned int, long int, unsigned long int if necessary for its value). It makes no difference.

      • Lundin says:

        #2, no I did not misunderstand his point, I think this is pretty clear in my post.

        #4 What misunderstanding of basic concepts? I wrote:
        “The main reason is that the C standard states that all variables that are global or static must be initialized before use. Either to the value the programmer initialized them to, or if they didn’t, to the value zero.”
        If you read this, you wouldn’t have needed to repeat the very same explanation in your own post to educate me about the things I had already written myself…

        My post explicitly mentions very common non-standard ways in embedded systems to skip mentioned problems with static initialization. Adapting the code towards this non-standard extension is what my whole text is about. If you read my post, I argue against using the C standard static initialization, since it is a bad thing for the reasons mentioned in my post. But most importantly, if you write standard C code that does not rely on static initialization, you can make your code insensitive to these non-standard extensions. This is highly relevant for embedded systems design!

        (Regarding those startup things that must be executed immediately, they are not necessarily time consuming. Of all the things I mentioned in my post, the clock initialization is the only one with delay potential, if you are using a PLL and not merely selecting some clock source or mode.)

        Regarding sloppy code: personally I always keep track of every variable of my program – there will be no variable in my programs that I don’t know what it does at any given time, it is a matter of discipline rather than coding style. I never “declare away” a number of variables that seem good to have and then write the program, I write the program and declare variables as I go, when needed. And I delete variable declarations when I find that they aren’t needed.
        Thus it is “no biggie” for me at least, I don’t initialize auto variables to zero “just in case I write a bug later on”. I don’t see how that would be defensive programming, I just consider it bad practice. To set everything to zero because you are counting on yourself to write bugs later on is not only strange, it will also effectively hide away those possible “later on” bugs, so that they won’t be caught during program development, static analysis or not. Instead they will live all the way to production code, perhaps completely disarmed by the zero-init, but bugs no less.
        Anyway, I can’t remember the last time I had a bug related to non-initialized auto variables. I don’t think I have written one of those since I was a beginner some 15+ years ago… and I have only used static analyzers in the later years, so at least to me this is a non-issue.

        • David Brown says:

          If you understand that the C toolchain will initialise static data (to specified values, or to 0) before the start of main(), then why do you go to such lengths to tell us that this is a bad thing, and to suggest it can be avoided by leaving out initialisers?

          Skipping this required initialisation is not “very common” – though it is undoubtedly non-standard. I know of only one tool that skips the zeroing of the bss – and it is not known as “Code Composter” for nothing. Any tool that does not require explicit settings to deviate from the standard behaviour is a broken tool. So you don’t need to write code that is insensitive to this “extension” – if you want to use such extensions, then you write code to suit it. If you want to write C code, then you write C code. It is certainly /not/ good to suggest that we all write code that is slower and often less clear, just in case someone wants to compile it with non-standard extensions.

          If you have something that must be done so quickly after power-on that it can’t wait for the initialisation of global data (yet, bizarrely, is happy to wait for reset generator to finish and a PLL to settle), then it should be handled in hardware using things like pull-up or pull-down resistors or other logic. Skipping the initialisation of ram will not fix a broken design.

          Incidentally, I am curious to your idea that “high integrity” systems forbid the reliance on RAM. How do these “high integrity” systems store run-time data? What sort of systems exist where you consider RAM so unreliable that you can’t risk initialising a variable at startup, but are happy to do so a few milliseconds later in code? “High integrity” means making sure your hardware suits the demands of the system, and that you have some sort of check that the hardware works. So if you have a situation where RAM is unreliable (such as high radiation environments), you use hardened RAM with ECC, and probably redundant systems and majority voting. You don’t try and write code on a system with the assumption that that system doesn’t work!

          Regarding “sloppy code” – I think you should re-read your original comment. I fully agree with you that you should not thoughtlessly initialise local variables as a way to try to avoid bugs – you should initialise variables to the value you want when you want to use them. (And of preference you should not define the variable until then, unless you are stuck with a pre-C99 compiler.) But what you wrote was “The programmer must make sure to set it at some point before using it. If they forget it… well, it is no biggie. Since all professional programmers use static analysers (right?), that will instantly find such simple bugs.” That sounds to me like you are suggesting it’s okay to write bad code and let the static analyser find the mistakes – rather than setting the variables value correctly when it is needed.

        • Petr says:

          Rule #2

          What are the dangers of integer promotions?
          Unsigned types are zero-extended (so 0xFF becomes 0x00FF), signed types are sign-extended (so -1 becomes -1), like one would expect.

          Rule #4 – Initialization at startup:

          A decent compiler (actually C runtime) has a nonstandard way to execute specified functions before this initialization is done.
          If C runtime does not allow that then editing linker scripts does the trick.
          If it is not possible and C runtime allows suppressing the initialization then your main can call the pre-init functions in C-runtime explicitly and then manually run function in C runtime to do the initialization.
          If user writes a function which relies on the initialization and there is a chance the init will not be done for some reason then the he can add an assertion to the code.
          Only few situations benefit from the pre-init code, it is foolish to want ordinary code to be prepared for such situation given the available solutions and diagnostic.

          Rule #4 & high-integrity:

          It is easier to do a full reboot then to re-run initialization of libraries.
          RAM usually contains larger amount of non-constant data then constant data, so reinitializing only constants will improve little reliability.

  20. Rule #4 has some merit.

    If you are coding for a microcontroller with a watch dog timer then your first line of code is usually to disable the WDT until you are ready to turn it back on. If your compiler startup code spends too much time initializing variables then the WDT interrupt can fire before your first line of code (turning off the WDT) is executed. I ran into this problem once and it was a bear to figure out what was behind the intermittant startup problem.

    Another valid reason for #4 is I prefer to have init functions that set all of the module level globals to a desired state. I sometimes call these init functions again as a way of performing a partial warm boot. It just makes sense to have initialization of these variables be under my direct control instead of happening behind the scenes.

    I also tend to prefer #5 as well. I often have used quite complicated #defines used to calculate register values for things like oscillator and timer registers based on human readable inputs (Crystal Speed, Clocks per Instruction etc). These calculations only need to be performed at compile time and it is pointless to do this at run time.

    Here is an example from an 8051 project I wrote years ago:

    // System Timer definitions
    #define OPCODES_PER_SECOND (CRYSTAL_SPEED/CLOCKS_PER_OPCODE)

    // Timer 0 reload defined to create 33 ms interrupt rate (roughly 30 ticks per second)
    #if (CLOCKS_PER_OPCODE == 12) // Compiled for emulator mode
    #define TIMER0_RELOAD (0xFFFF – TICK_RATE_MILLISECONDS * OPCODES_PER_SECOND/2000L)
    #else
    #define TIMER0_RELOAD (0xFFFF – TICK_RATE_MILLISECONDS * OPCODES_PER_SECOND/1000L)
    #endif

    #define LOW_BYTE(w) ((w)&0xff)
    #define HIGH_BYTE(w) (((w)>>8)&0xff)

    Later I use this #defined value in the interrupt routine

    //Reload Real time clock tick rate
    TH0 = LOW_BYTE(TIMER0_RELOAD);
    TL0 = HIGH_BYTE(TIMER0_RELOAD);

    Sure I could have just hard coded the values, but if my processor speed changed I would have to manually recalculate the timer reload values. This way all I have to do is change the CRYSTAL_SPEED definition and everything that is effected by a change in crystal speed (baud rate generator, timer init value etc) are automatically set to the correct values.

  21. _d says:

    You may do the same things with enums instead of macro definitions:
    enum
    {
    //…
    TIMER0_RELOAD = 0xFFFF – TICK_RATE_MILLISECONDS * OPCODES_PER_SECOND/1000L,
    TL0 = HIGH_BYTE(TIMER0_RELOAD),
    // .. etc
    };

    Only function-like HIGH_BYTE() and LO_BYTE() in your example need to be a macro. However, in C++0x you may use constexpr functions instead.

  22. Laz says:

    Regarding #5, some of you are refering to values within one source module, and others are refering to values that are shared across the project, including compiling options. You can’t use a static const across modules. You can use encapsulation methods, but isn’t that getting extreme?
    For example, I would use #define PI (3.1415f) and put that in constants.h, which is included in all modules. As stated here, I should create a static const in constants.c, containing a function called pi(), which would have a prototype shared among all modules. A similar situation for system clock parameters. Possibly more correct, but that’d be diffcult for me to adopt.
    I only use enums for lists where the value doesn’t matter, eg colors, or indices, and I use the resulting type checking to make sure I don’t cross-use them. Also useful for auto-sizing arrays.
    #define for bits always, for reasons described above. These should be local to a module/driver anyway.

    • David Brown says:

      You can happily put “static const” definitions in a header file. Unless you do something odd like take the address or change it’s value, the result will be at least as code space, data space and time efficient as using a #define value.

      It’s just like using “static inline” functions in a header instead of function-like macros.

      • Eric Wertz says:

        I have to totally disagree — my empirical tests with GCC don’t bear this out at all, at least not with the default options. I will concede that there *might* be a set of compiler switches that may mitigate some or all of these space/time artifacts (individually, or maybe together), but I’m extremely skeptical that using all of these constructions interchangeably “A Good Thing(tm)” in the vast majority of situations.

        In fact I contend that specifying “static const” as you state is almost the worst construction one can come up with from a space and time standpoint. With GCC, one gets space allocated for every datum specified as “static const” in EVERY module that includes such a header file, whether any of declared data are used or not. In addition to the data space penalty, one also gives up the ability to encode the constant as an immediate operand in the instruction for small enough constants by removing their lexical “constantness” at the site to be compiled.

        In the end, it all comes down to what the world’s collective (if you care about portability) legitimate compilers do today. I don’t doubt that the very, very best compilers can minimize the impact of any arbitrary, abstract construction — AND if the toolchain has complete information about the entirety of the code to be linked together — but I think that that’s by far the exception rather than the rule. Some of these artifacts often can’t be mitigated by anything but a very sophisticated tool chain.

        • David Brown says:

          What do you mean “the default options”? No one uses a compiler without any options – certainly not for embedded development. So if you use gcc (or any other compiler) without enabling any optimisation flags, you get what you asked for – the simplest, big, slow code generated from the most literal interpretation of the source code.

          If you use the compiler as it is designed – it’s an optimising compiler, so enable the optimiser – then the “static const” will produce the same code, or perhaps better, as #define (including doing constant folding, using immediate operands, using different instructions, pre-calculating the results, etc.). Non-static “const” will often produce similar code, since gcc assumes that the programmer follows the rules of C (some compilers are more pessimistic), but it will additionally generate space for the constant in ram or flash.

          Try it and see – most embedded programs are compiled with -Os, but you can use -O1 or -O2 if you prefer, and use -Wa,-adchls to generate a listing file.

          It is true that few compilers are as good at optimising as gcc, and many will generate poorer code for the non-static const since they allow for the possibility that another module will change the value. But if your compiler won’t optimise a “static const” properly (when you’ve enabled optimisation, of course), then it really is time to get better tools – or to accept that your old tools generate slow code.

  23. Mac H says:

    Bad Rule # 3 (His ‘good rule # 7’) is not only a bad idea – it is actually a terrible bug.

    Consider this example:

    //
    // If we have enough of the life saving drug, administer it to save the patient’s life. Otherwise the patient will die.
    //
    if (AmountOfLifeSavingDrug >=MinimumAcceptableLevel)
    {
    AdministerDrugAndSavePatientsLife(AmountOfLifeSavingDrug);
    }

    According to this bizarre bug inducing ‘rule’, we should optimise it to be:

    //
    // Allegedly ‘optimised’ (but fatally buggy) version which will kill the patient
    //
    if (AmountOfLifeSavingDrug >MinimumAcceptableLevel-1)
    {
    AdministerDrugAndSavePatientsLife(AmountOfLifeSavingDrug);
    }

    It looks fairly similar (if a little bizarre) – but it can kill the patient.

    Imagine the simplest possible case ; ‘AmountOfLifeSavingDrug’ & ‘MinimumAcceptableLevel’ are unsigned integers. (I’ll assume 16 bit integers for the sake of the example – but the logic works for signed as well – any size)

    So, if we wish it to accept any level, then we set ‘MinimumAcceptableLevel = 0’ (Or, if the values were signed, MIN_SIGNED_INT)

    In the correct (and intended) version, this is:

    if (AmountOfLifeSavingDrug >=0) <– Always true
    {
    AdministerDrugAndSavePatientsLife(AmountOfLifeSavingDrug); 65535u) <– Always false.
    {
    AdministerDrugAndSavePatientsLife(AmountOfLifeSavingDrug); <– Patient Killed
    }

    —–

    Overall the advice in that blogpost was riddled with basic errors. (eg: 'char' is not '-128 to +127' .. it is processor (and compiler!) dependant. It could be unsigned. It could even (on DSPs) be 32 bits – the C standard permits it)

    It was originally ten rules but it is down to eight now that errors have been pointed out.

    I only hope that more will be eliminated before anyone starts taking this seriously.

    Mac

  24. Colin Walls says:

    I am unable to find the original posting, but I have to agree with just about every word you say here. I am rather frightened to hear that someone developing medical instruments has such perverse ideas.

  25. Rubberman says:

    Rule #1 – right on! Shifting for integer multiplication or division by 2 can by useful when every machine cycle counts, but different processors handle wrapping and overflows/underflows differently. Caveat programmer!
    #2 – ditto – always use appropriately typed variables – an int can be anything from 8 to 64 bits, depending upon the processor and compiler. Bad assumptions make for bad code.
    #3 – a good example of programming by intention. Make your code clear what your intent was. Even if you have to maintain it yourself, it will help you remember what you meant in a particular situation.
    #4 – totally agree! My rule is to initialize all variables as close to the declaration (preferably in the declaration) as possible. An assignment takes processor time. Initialization in a declaration instance, especially if the value is a constant, can be handled by the compiler, hence has zero run-time overhead.
    #5 – using #defines instead of const typed global/local variables is old school for most of this. There are exceptions, but I try to stay away from the #define structure when possible. The downside? The const variables do take up memory, whereas #defines do not (generally). Each construct has its own uses. Just make sure they are appropriate for your situation.

    • David Brown says:

      Rule #1 – if every cycle counts, get a compiler that does a decent job (or learn how to use the compiler you have). Don’t do silly hand-optimisations like this. Write your source code in a clear logical manner, and let the compiler generate optimal implementations. Compilers can do a better job of optimisation if you have written sensible code, rather than such half-way optimisations. You do your job, and let the compiler do it’s job.

      Rule #2 – No one has argued against using appropriately sized types – just against using C types like “char” and “short” instead of explicitly sized types. Read the article!

      Rule #4 – The compiler will still need to generate code to initialise a variable – you don’t get something for nothing. But if you have a global (or static) variable and initialise it in code rather than at the definition (not the declaration), then the target needs to do twice the work – it must first clear the variable to 0, then initialise it in the code.

      Rule #5 – Again, for everyone who has failed to read the other comments, “static const” do not take up memory. I wish Michael Barr had got this right in his post, so that we wouldn’t have to go through it again and again.

      • Ivousch says:

        Rule #1 does not belong into coding rules. But as it was mentioned above it belongs to optimization techniques. I believe every embedded programmer should be aware about this technique and it seems everyone here around know it anyway.

        No compiler can optimize fixed point arithmetic if it is badly written, here is an example to calculate circle area with fixed point:

        uint32_t a,r;
        a = (((uint32_t)(3.14 * 100))*r*r)/100;

        To get the right level of optimization from your compiler you have to write:
        a = (((uint32_t)(3.14 * 128))*r*r)/128;

        This won’t harm even if you are using CPU architecture where division takes only 1 cycle and it will boost 8-bits micro a lot.

  26. Bill Collis says:

    It may be really easy for compiler writers to detect such “power of two” divisions and replace them with shift, but they don’t do this. and should not do so because the result is not the same. Let’s suppose that we want to divide an int by 2. If the value happens to me -1 then division will give 0 and a shift right arithmetic will give -1. Maybe this matters. Maybe it doesn’t. But no compiler should assume it doesn’t matter.

    Personally I don’t find any of the 5 rules “dangerous” . Rather, I felt them to be irrelevant to the real problem of writing reliable code. Some of the examples of bugs given above are not appropriate because you can still apply the rules without bugs. Writing deliberately buggy code and blaming the “rules” is not helpful. But is this always true?

    The trouble with so called coding standards is that they are a kind of unverified dogma. There is no assurance at the end of the day that the restrictions placed on the poor coder really translate into superior code. Most of the “rules” are far too widely drawn and it is quite possible that the efforts to comply actually lead to buggy code.

    Example: MISRA says you cannot compare a float for equality with a constant. So one of my staff didn’t test for division by 0.0! The rules should allow comparison with small integers which have exact floating point representations.

    I take issue also with David Brown who claims that “static const” do not take up memory. If that were true you could not calculate its address as the standard demands. It would be a miracle if all my error messages didn’t take up any memory!

    • David Brown says:

      “static const” data is not allocated any memory under normal circumstances, for the sort of usage being described here as a replacement for #define’d numeric constants. Of course a “static const” will be allocated memory if you take its address (unless the complier can optimise the code to avoid it) – that’s just like local variables being held in registers or “optimised away” unless you take their addresses and force them onto the stack. And as I said before, “static const” will likely take memory if you fail to enable your compiler’s optimiser.

      MISRA has lots of very poor rules, but “do not compare a float for equality” is a good one. It is very bad practice to test for equality to 0.0 before a division – the correct handling is to write your code so that the code path in question is not run with invalid data. It is certainly wrong to think that testing for a denominator of 0.0 will somehow magically make your code work – you can be pretty sure that a very small denominator will cause an overflow with equally bad results. So if you are unable to ensure that you don’t call that code with invalid data, your test will really be something like “if (abs(x) < 0.0001) {…}".

      You are quite correct that following coding standards does not give any guarantee or assurance that code is correct. But it does give a guarantee that certain classes of bugs are very unlikely, and it gives a guarantee of consistency that makes it easier to avoid or spot many other types of errors.

      You are also correct that poor coding standards can result in worse code as the programmer is forced to follow the standard. This is the case with MISRA – it has many rules that, IMHO, lead to worse code.

      • Lundin says:

        Everything takes up memory, no numbers can be allocated in thin air. The optimization you describe will merely move the constant from the const segment of NVM or RAM into the program code, where it will reside together with the op codes. The total amount of used memory data+program will be the same.

        Regarding the MISRA-C rant, I agree that MISRA is harmful for junior programmers. If you didn’t know that you can’t compare a float with equality operators, then you aren’t competent enough to comprehend the quite advanced MISRA document. You must have at least one professional senior programmer in the team to use MISRA-C.
        For professional programmers it is an excellent set of rules, save for a few strange ones that hopefully will be fixed in the upcoming MISRA C3.

        • David Brown says:

          No, you are wrong about the memory use of “static const” (or #define’d values). When then compiler can see these as “immediate” compile-time constants, they are almost always more efficient than a constant value at a particular address in memory. When the compiler wants to use such a value, it can load it directly into a register or even use it as an immediate operand in an assembly operation (depending on the target cpu and the type of value). The value will take space as part of the program code – that much is correct. But if the value is in memory as an individual const, then the compiler must generate an operation to load it into a register from memory – that means that the address of this const is included in the program code, while the const itself is in data memory (ram or flash). You have approximately doubled the memory cost (depending on the target), and almost certainly slowed the code.

          On top of that, there is the missed optimisation opportunities that could make a much bigger difference.

          When doing embedded programming, if you are at all interested in the efficiency (time or space) of your generated code, then it is important to understand the architecture of the target cpu, and to take time to study the compiler’s generated assembly code for different types of source code. It is much more effective than making up rules based on “common sense”.

          I’m sure you are able to help your junior programmers write reasonable code with MISRA. For my own part, I will continue to flaunt MISRA rules when it leads to clearer, safer code, or more efficient code without sacrificing clarity or safety.

    • Paul Curtis says:

      Some compilers do replace divisions by right shifts, even for integers, by using an addition beforehand. Try it with gcc. And then try dividing one integer by a constant and find a magic multiplication instead. Try Hacker’s Delight and the wealth of compiler material about this.

  27. Tim Wescott says:

    Another hit against Rule #1 (if it hasn’t been mentioned), is that per the C standard when you right-shift a signed integer with a negative value, the results are undefined.

    You may get an arithmetic shift, so that (-4) >> 1 becomes -2, or you may get a logical shift, so that (-4) >> 1 becomes 32766. One of those results is, of course, far less embarrasing than the other when it comes time to test things out in the lab.

    I’ve been bitten by this. Fortunately, it was on a compiler that would see / (power of 2 int) and turn it into a nice fast arithmetic shift.

  28. Glenn Hamblin says:

    Just wanted to make a quick note on Bad Rule #4. I agree that it is a bad rule, but I once used a manufacturer specific compiler for an embedded device that would make any variable that was assigned a value at declaration a constant. Wow, was that hard to adjust to. Perhaps BadAdvice had to deal with that same controller or something similar in the past?

    Obviously this would not be the case in any standard C compiler, but there are still a number of odd ball device specific compilers out there that don’t act the way you would expect.

  29. I would recommend using MISRA C as the basis for code development and a compiler that checks for compliance. This is how I train my students and follow myself. It is a strong basis for good coding.

  30. Mark Perry says:

    This is a really good response. Rule #1, in particular, besides ignoring a good compiler’s ability to pick optimal instructions for a given architecture, also ignores the fact that quite a few modern microcontrollers, and virtually all modern microprocessors, have fast, hardware-implemented integer division instructions. In some instances a shift implementation might actually be slower–or the compiler might outsmart the programmer and implement the shift as a hardware divide.

    Regarding rule #4 I confess I actually kinda like this rule, stated differently and for a completely different reason than the one given by BadAdvice. First, though, I want to clarify that (like Mr. Barr if I’m reading him correctly) I would much prefer to delay variable definition until as close as possible to variable usage. To me this is best for clarity and probably also for performance. For the rest of my post, however, I assume I’m writing C90-compliant code and have to define my variables at the top of the function body, because I have frequently have had to do just that.

    To me, initializing auto variables at definition (assuming you can’t postpone definition) is often less clear because it can obscure the meaning of the initialization value, and/or that value’s impact on how the variable is used. I point to the C “for” construct as an example of what I mean, where it is common to see something like “for (Index=0; Index < MAX_INDEX; Index++)". The variable Index (declared at the top of the function body in a C90-enforcing compiler) is initialized in the "for" construct because that's where it is used, and because the meaning of the initialization value is tied to the usage of the variable as an index variable, or representing some other iterative series of values. Wouldn't it be awkward to see:

    uint32_t Index = 0u;

    for (; Index Head;
    }
    else {
    e = NULL;
    }
    return e;
    }

    clearly communicates the meaning of the initialization value NULL given to e, whereas this:

    Queue_Elem* Queue_Peek(const Queue *q)
    {
    Queue_Elem *e = NULL;

    if (!Queue_Empty(q)) {
    e = q->Head;
    }

    return e;
    }

    risks hiding the meaning behind initializing e to NULL.

    This is probably too trivial to really illustrate my point, but imagine the second example as a more complicated function with more auto variables defined and a fair amount of code between the definition+initialization of e, and e being assigned the value of q->Head inside the conditional. It’d be easy to look at the variable’s usage, being unable to simultaneously see the initialization value of e, and wonder “what is supposed to happen if q is empty?” You’d have to scroll back up to the variable declarations to find the answer.

    If you refactor the first version of the function to, say, make e into a call-by-reference parameter and return instead an enumeration value to indicate success or failure, it’d be pretty easy to remember to initialize e but forget the meaning of the initialization value–e gets NULL when q is empty–and more easily introduce the logical error of forgetting to properly handle the q empty case. For example:

    enum Queue_Result {
    Queue_Success,
    Queue_Empty,
    Queue_Uninitialized,

    };

    enum Queue_Result Queue_Peek(const Queue* q, Queue_Elem *e)
    {
    enum Queue_Result result = Queue_Success;
    e = NULL;

    if (!Queue_Empty(q)) {
    e = q->Head;
    }

    return result;
    }

    The variables are all initialized and there’s no compiler warning, but the logic is faulty. Of course, this is easily avoided or fixed by initializing “result” with “Queue_Empty” instead of “Queue_Success,” essentially duplicating the meaning conveyed by initializing e to NULL in the original function, but my point is that I think the error is easier to avoid if e is initialized at usage, where the meaning is more obvious. The refactor probably looks like:

    enum Queue_Result Queue_Peek(const Queue* q, Queue_Elem **e)
    {
    enum Queue_Result result;

    if (!Queue_Empty(q)) {
    *e = q->Head;
    result = Queue_Success;
    }
    else {
    *e = NULL;
    result = Queue_Empty;
    }

    return result;
    }

  31. Mark Perry says:

    Whoops! my post got “clipped” in the middle, somehow.

    Here’s what got “clipped,” book-ended with what is in the post…

    …Wouldn’t it be awkward to see:

    uint32_t Index = 0u;

    for (; Index Head; Index++) {

    }

    I would argue that:

    Queue_Elem* Queue_Peek(const Queue *q)
    {
    Queue_Elem *e;

    if (!Queue_Empty(q)) {
    e = q->Head;
    }
    else {
    e = NULL;
    }
    return e;
    }

    clearly communicates the meaning of the initialization value NULL given to e, whereas this:

    Queue_Elem* Queue_Peek(const Queue *q)
    {
    Queue_Elem *e = NULL;

    if (!Queue_Empty(q)) {
    e = q->Head;
    }

    return e;
    }

    risks hiding the meaning behind initializing e to NULL.

    …I hope this clarifies my post.

  32. Phil Ouellette says:

    Rule #5 can have application for microcontrollers with watch dog timer reset circuitry. I once designed a product around an 8051 variant. This particular processor had a watch dog timer reset that was Enabled coming out of reset.

    The first line of C code in my firmware turned off the watch dog timer, however I had a lot of initialized variable data defined. During the development, the point came where the time consumed by the variable initialization code exceeded the default watch dog timeout and the system would intermittently hang coming out of reset.

    Simplest answer was to add a line of code to the startup.a51 module to disable the watch dog timer reset then jump to the normal C initialization code. It did make me reconsider the amount of preprocessor initialized code I used. In subsequent designs I started using more OO style coding and included constructor functions that explicitly initialized each modules variables.

    You can write perfectly fine OO code in C, it just doesn’t help you do so.

  33. Richard Haven says:

    “Trying to outsmart a compiler defeats much of the purpose of using one.” — Kernighan & Plauger

  34. Slipoch says:

    It sounds like he is an assembler programmer who has moved up to C.
    Especially the right shift instruction.
    Traditionally Assembler was very quick in comparison to C, particularly with mathematical calculations if you had to do them many times per second. Less overheads of memory and cpu cycles etc etc. HOWEVER there is a major difference between then and now. Nowadays we have compilers written to such an extent that they could almost seem prescient in the way they interpret code and find the best solution.

  35. Domenik He says:

    Introducing: In general your ‘review’ leaves out a fact of the scope BadAdvices Rules are ment for.
    It is for embedded systems, where it even might happen you are challanged with just having 2KB of memory or slow/frequently repeated invokations where every clockcycle that can be droped is worth the performance. All of his advices are bad (at least the way you present them), as they are absolutley too much generalized. Also the linked book describes it self as laying priority in maintainability and readability where as the given rules are absolutly against it and high performance optimizations.
    Anyway for the given scope of the book, as bad as his advices are, whilest your are the better in general desktop enviroments or embedded systems with enough system resources, yours are bad advices for the given scope of the book aswell. Since you just say to ignore his advices which are as core correct. BUT the author of that book is not able to transport the idea to the reader. But your 4th better advice is at all as bad as his advices are.

    “Better Rule: Declare constants using const or enum.”

    -> This would just kill the advantage of defines don’t need memory. why should I do “const double NUM_PI = 3.14” if I could do “#define NUM_PI 3.14” aswell?

    my better advice would be: use defines for constants as it is an value you refer to multiple times in your code and/or it could change in a later revision so it can easily be changed. BUT you dont need that value to be refered to by a pointer.
    Because: Why should you place an constant value into the memory, if you don’t need it to be in the memory? Also file size would not change, since in either way it had to be written into the file. Also you don’t consider the fact that a define is valid on file scope, while the scope of an const is depending on its declarations scope.

    otherwise a const is usefull in case u need the value to be referable by adressof operator or you want to mess around with its types attributes like size by sizeof operator (so ofcourse a define wouldn’t fit as for its types attributes the value has to be identifed through a type). Or… for what ever reason in case you want to limit the scope of the value. But I really couldn’t imagine a case you want this.

    So at least for this Rule, your advice is as bad as his advice since you oversimplify in the same way he does.

    Also note:

    “A short integer may be 16-bits or 32-bits (or something else), depending on the compiler;” -> but by standard anyway not less than 16 bit!

  36. Jeff Kelley says:

    Bad Rule #4: Avoid variable initialization while defining.

    Uninitialized variables are great because the compiler will generate a warning that points out the mistake.
    On the other hand, initializing when defining just to avoid the problem will in fact hide it.

  37. Adrian Popa says:

    With Better Rule #5 I 100% disagree.
    1st the enum size is not fixed. You can change it from compile option, so supposedly you have to link a static library compiled with 32 bits enum size while your enum size is 16 bits will fails (at least on Arm’s RVDS compiler). So better use defines if the goal is to provide a compiled library not the code.
    2nd const is a insurance given to the compiler. You can always change that value by casting it into a pointer.
    3rd the const data is placed in the memory based on linker script rules (it can be in ram and unnecessary copy from flash to ram or in flash and any attempt of 2nd may end in a fatal interrupt)

    I whould add as rule “take care at sizeof usage”. For example a USB CDC ACM device have (classic) configuration descriptor of 67 bytes. If is packed as structure the structure will contain padding, so sizeof will return useful data + padding. On Windows, Linux, Mac OS should not cause issues, but in case of an embedded host ….

Leave a Reply