This is the eighth in a series of tips on writing effective C. Today’s topic concerns how best to compare two structures (naturally of the same type) for equality. The conventional wisdom is that the only safe way to do this is by explicit member by member comparison, and that one should avoid doing a straight bit (actually byte) comparison using memcmp() because of the problem of padding bytes.

To see why this argument is advanced, one must understand that a compiler is free to place pad bytes between members of a structure so as produce more favorable alignment of the data in memory. Furthermore, the compiler is not obligated to initialize these pad bytes to any particular value. This code fragment illustrates the problem:

typedef struct
{
 uint8_t x;
 uint8_t pad1;  /* Compiler added padding */
 uint8_t y;
 uint8_t pad2;  /* Compiler added padding */
} COORD;

void foo(void)
{
 COORD p1 p2;

  p1.x = p2.x = 3;
  p1.y = p2.y = 4;
  /* Note pad bytes are not initialized */
  if (memcmp(&p1, &p2, sizeof(p1)) != 0)
  {
    /* We may get here */
  }
 ...
}

Thus, it’s clear that to avoid these kinds of problems, we must do a member by member comparison. However, before you rush off and start writing these member by member comparison functions, you need to be aware of a gigantic weakness with this approach. To see what I mean, consider the comparison function for my COORD structure. A reasonable implementation might look like this:

bool are_equal(COORD *p1, COORD *p2)
{
 return ((p1->x == p2->x)  &&  (p1->y == p2->y));
}

void foo(void)
{
 COORD p1 p2;
 p1.x = p2.x = 3;
 p1.y = p2.y = 4;

 if (!are_equal(&p1, &p2))
 {
   /* We should never get here */
 }
 ...
}

Now consider what happens if I add a third member z to the COORD structure. My structure definition and function foo() become:

typedef struct
{
 uint8_t x;
 uint8_t pad1;  /* Compiler added padding */
 uint8_t y;
 uint8_t pad2;  /* Compiler added padding */
 uint8_t z;
 uint8_t pad3;  /* Compiler added padding */
} COORD;

void foo(void)
{
 COORD p1 p2;
 p1.x = p2.x = 3;
 p1.y = p2.y = 4;
 p1.z = 6;
 p2.z = 5;

 if (!are_equal(&p1, &p2)
 {
  /* We will not get here */
 }
 ...
}

The problem is that I now have to remember to also update the comparison function. Now clearly in a simple case like this, it isn’t a big deal. However, in the real world where you might have a 500 line file, with the comparison function buried miles away from the structure declaration, it is way too easy to forget to update the comparison function. The compiler is of no help. Furthermore it’s my experience that all too often these sorts of problems can exist for a long time before they are caught. Thus the bottom line, is that member by member comparison has its own set of problems.

So what do I suggest? Well, I think the following is a reasonable approach:

1. If there is no way that your structure can change (presumably because of outside constraints such as hardware), then use a member by member comparison.
2. If you are working on a system where structure members are aligned on byte boundaries (which is true to the best of my knowledge for all 8 bit processors, and also most 16 bit processors), then use memcmp(). However, you need to think about doing this very carefully if there is the possibility of the code being ported to a platform where alignment is not on an 8 bit boundary.
3. If you are working on a system that aligns on a non 8 bit boundary, then you must either use member by member comparison, or take steps to ensure that all the bytes of a structure are initialized using memset() before you start assigning values to the actual members. If you do this, then you can probably use memcmp() with a reasonable amount of confidence.
4. If speed is a priority, then clearly memcmp() is the way to go. Just make sure you aren’t going to fall into a pothole as you blaze down the road.

Before I leave this topic, I should mention a few esoteric things for you to consider.

If you use the memcmp() approach you are checking for bit equality rather than value equality. Now most of the time they are the same. Sometimes however, they are not. To illustrate this, consider a structure that contains a parameter that is a boolean. If in one structure the parameter has a value of 1, and in the other structure it has a value of 2, then clearly they differ at the bit level, but are essentially the same at a value level. What should you do in this case? Well clearly it’s implementation dependent. It does however illustrate the perils of structure comparison.

Finally I should mention issues associated with structures that contain pointers. CS guys like to distinguish between deep and shallow structure comparison. I rarely write code where a deep comparison is required, and so for me it’s mostly a non-issue.

Next Tip

Previous Tip
Home

This is the seventh in a series of tips on writing effective C. Today’s topic concerns function parameters, and more to the point, how you should choose them in order to make your code considerably more resilient to parameter passing errors.  What do I mean by parameter passing errors? Well consider a function that is intended to draw a rectangle on a display. The lousy way to design this function interface would be something like this:

void draw_rect(int x1, int y1, int x2, int y2, int color, int fill)
{
...
}

I must have seen a function like this many times. So what’s wrong with this you ask? Well in computer jargon the parameters are too weakly typed. To put it into plain English, it’s way too easy to pass a Y ordinate when you are supposed to pass an X ordinate, or indeed to pass a color when you are supposed to be passing an ordinate or a fill pattern. Although in this case (and indeed in most cases) these types of mistakes are clearly discernible at run time, I’m a firm believer in catching as many problems at compile time as possible. So how do I do this? Well there are various things one can do. The most powerful technique is to use considerably more meaningful data types. In this case, I’d do something like this:

typedef struct
{
 int x;
 int y;
} COORDINATE;

typedef enum
{
 Red, Black, Green, Purple .... Yellow
} COLOR;

typedef enum
{
 Solid, Dotted, Dashed .. Morse
} FILL_PATTERN;

void draw_rect(COORDINATE p1, COORDINATE p2, COLOR color, FILL_PATTERN fill)
{
...
}

Now clearly it’s highly likely that your compiler will complain if you attempt to pass a coordinate to a color and so on – and thus this is a definite improvement. However, nothing I’ve done here will prevent the X & Y ordinates being interchanged. Unfortunately, most of the time you are out of luck on this one – except in the case where you are dealing with certain sizes of display panels with resolutions such as 320 * 64, 320 * 128 and so on. In these cases, the X ordinate must be represented by a uint16_t whereas the Y ordinate may be represented by a uint8_t. In which case my COORDINATE data type becomes:

typedef struct
{
uint16_t x;
uint8_t y;
} COORDINATE;

This will at least cut down on the incidence of parameters being passed incorrectly.

Although you probably will not get much help from the compiler, you can also often get a degree of protection by declaring appropriate parameters as const. A good example of this is the standard C function memcpy(). If like me, you find yourself wondering if it’s memcpy(to, from) or memcpy(from, to), then an examination of the function prototype tells you all you need to know:

void *memcpy(void * s1, const void * s2, size_t n);

That is, the first parameter is simply declared as a void * pointer, whereas the second parameter is declared as void * pointer to const. In short the second parameter points to what we are reading from, and hence memcpy is indeed memcpy(to, from). Now I’m sure that many of you are thinking to yourself – so what, the real solution to this is to give meaningful names to the function prototype. For example:

void *memcpy(void *destination, const void *source, size_t nos_bytes);

Although I agree wholeheartedly with this sentiment, I’ll make two observations:

1. You are assuming that the person reading your code is sufficiently fluent in the language (English in this case) that the names are meaningful to them.
2. Your idea of a meaningful label may not be shared by others. I’ve noticed that this is particularly the case with software, as it seems that all too often the ability to write code and the ability to put a meaningful sentence together are inversely correlated.

The final technique that I employ concerns psychology!  Now one can argue that the failure to pass parameters correctly is due to laziness on behalf of the caller. At the end of the day, this is indeed the case. However, I suspect that in many cases, it’s not because the caller was lazy, but rather it’s because the caller thought they knew what the function parameter ordering is (or should be). A classic example of this of course concerns dates. Being from the UK (or more relevantly – Europe), I grew up thinking of dates as being day / month / year. Here in the USA, they of course use the month / day / year format. Thus when designing a function that needs to be passed the day, month and year, in what order should one declare the parameters? Well in my opinion it’s year, month, day. That is the function should look like this:

void foo(int16_t year, MONTH month, uint8_t day)

There are several things to note:

1. By putting the year first, one causes both Europeans and Americans to think twice. This is where the psychology comes in!
2. I’ve made the year signed – because it can indeed be negative, whereas the month and day cannot.
3. I’ve made the month a MONTH data type, thus considerably increasing the likelihood that an attempt to pass a day when a month is required will be flagged by the compiler.
4. I’ve made the day yet another data type (that maps well on to its expected range). Furthermore, attempts to pass most year values to this parameter will result in a compilation warning.

Thus I’ve used a combination of psychology and good coding practice to achieve a more robust function interface.
Thus the bottom line when it comes to designing function interfaces:

1. Use strongly typed parameters.
2. Use const where you can.
3. Don’t assume that what is ‘natural’ to you is ‘natural’ to everyone.
4. Do indeed use descriptive parameter names – but don’t assume that everyone will understand them.
5. Apply some pop psychology if necessary.

I hope you find this useful.

Next Tip

Previous Tip

Home

This is the fifth in a series of tips on writing what I call effective C.

Today I’d like to offer a simple hint that can potentially make your buffer manipulation code a little more robust at essentially zero cost. I’d actually demonstrated the technique in this posting, but had not really emphasized its value.

Consider, for example, a receive buffer on a communications channel. The data are received a character at a time under interrupt and so the receive ISR needs to know where to place the next character. The question arises as to how best to do this? Now for performance reasons I usually make my buffer size a power of 2 such that I can use a simple mask operation. I then use an offset into the buffer to dictate where the next byte should be written. Code to do this typically looks something like this:

#define RX_BUF_SIZE (32)
#define RX_BUF_MASK  (RX_BUF_SIZE - 1)
static uint8_t Rx_Buf[UART_RX_BUF_SIZE]; /* Receive buffer */

static uint8_t RxHead = 0; /* Offset into Rx_Buf[] where next character should be written */

__interrupt void RX_interrupt(void)
{
 uint8_t rx_char;

 rx_char = HW_REG;         /* Get the received character */
 Rx_Buf[RxHead] = rx_char; /* Store the received char */
 ++RxHead;                 /* Increment offset */
 RxHead &= RX_BUF_MASK;    /* Mask the offset into the buffer */
}

In the last couple of lines, I increment the value of RxHead and then mask it, with the intention of ensuring that the next write into Rx_Buf[] will be in the requisite range. The operative word here is ‘intention’. To see what I mean, consider what would happen if RxHead gets corrupted in some way. Now if the corruption is caused by RFI or some other such phenomenon then you are probably out of luck. However, what if RxHead gets unintentionally manipulated by a bug elsewhere in your code? As written, the manipulation may cause a write to occur beyond the end of the buffer – with all the attendant chaos that would inevitably arise. You can prevent this by simply doing the masking before indexing into the array. That is the code looks like this:

__interrupt void RX_interrupt(void)
{
 uint8_t rx_char;

 rx_char = HW_REG;         /* Get the received character */
 RxHead &= RX_BUF_MASK;    /* Mask the offset into the buffer */
 Rx_Buf[RxHead] = rx_char; /* Store the received char */
 ++RxHead;                 /* Increment offset */
}

What has this bought you? Well by coding it this way you guarantee that you will not index beyond the end of the array regardless of the value of RxHead when the ISR is invoked. Furthermore the guarantee comes at zero performance cost. Of course this hasn’t solved your problem with some other piece of code stomping on RxHead. However it does make finding the problem a lot easier because your problem will now be highly localized (i.e. data are received out of order) versus the system crashes randomly. The former class of problem is considerably easier to locate than is the latter.

So is this effective ‘C’. I think so. It’s a simple technique that adds a little robustness for free. I wouldn’t mind finding a few more like it.

Next Tip
Previous Tip
Home