Dark Corners of C - The Comma Operator

Stephen FriederichsJuly 23, 20158 comments

I've been programming in C for 16 years or so and the language has existed for much much longer than that. You might think that there'd be nothing left to surprise me after so long - but you'd be wrong. Imagine my surprise the first time I saw a line of code that looked something like this:

if (!dry_run && ((stdout_closed = true), close_stream (stdout) != 0))

My mind couldn't parse it - what's a comma doing in there (after (stdout_closed = true))?  I'd never seen anything like it in C before. I was sure it was a mistake, a typo or someone's idea of a funny joke with the preprocessor. Amazingly enough, it was none of those things: it was completely valid vanilla C. This was my introduction to the comma operator.

The comma operator is a fundamental operator in C that doesn't get nearly the exposure that other, more often used operators such as '=' or '++' get. Its purpose is to join together expressions into one line in a very specific way: the expression to the left of the comma is first evaluated and the result is ignored, then the expression to the right is evaluated and the result is returned. 

At first glance it doesn't seem to be that useful of an operator - who needs to put two expressions on the same line and ignore the outcome of one of them? Judging from the lack of common knowledge of the comma operator it seems most people get along fine without it, so what is a good use case for it?

The Wikipedia article has a few rather good use cases for it that you should read. The upshot is that it can be helpful in grouping expressions together in code to improve the readability of the code. In my opinion this is a slight advantage that introduces its own disadvantages. I'll illustrate with an example.

While I don't remember when I first saw the comma operator for the first time, it came back into the forefront of my mind when I was working on writing a circular buffer library for an upcoming article. Circular buffers involve reading and writing lots of memory which means lots of loops. A typical memory copy (for a linear buffer) looks something like this:

void memcpy(void * dest, void * src, size_t len)
{
    int i;
    for(i=0;i<len;i++)
        dest[i] = src[i];
}

This function relies on the fact that the source buffer is linear. The next memory address is always right after the current one : all you need to do to get there is increment. A circular buffer doesn't work the same way because it's topology is (obviously) a circle: the source pointer can start off anywhere within the buffer and will wrap back to the start of the buffer when it reaches the end. This makes it more complicated to write a memory copy out of a circular buffer - it might look something like this:

void cbRead(cbRef buffer, void * dest, size_t len)
{
    int i;
    for(i=0;i<len;i++)
    {
        dest[i] = buffer.data[buffer.srcPtr++]
        if(buffer.srcPtr == buffer.length-1)
            buffer.srcPtr = 0;
    }
}

(Note that this code isn't complete by any means - at the very least there's no check to ensure there's enough data in the buffer). 

In the circular buffer case, the buffer maintains a source pointer that indicates what element inside of its data array has the first byte of data. This pointer has to be wrapped back around to the beginning of the buffer by the if  statement to respect the circular nature of the buffer. 

This implementation looks clunkier than the linear buffer memory copy and with that conditional inside the loop, I'd guess it's slower as well. You can replace the conditional statement with something much quicker if you are willing to live by the following restrictions:

  1. Every buffer has to be the same, fixed size
  2. That size is a power of 2 minus one (example: 31 is 2^5 -1)

These restrictions allow you to use bit masking to replace the if statement:

void cbRead(cbRef buffer, void * dest, size_t len)
{
    int i;
    for(i=0;i<len;i++)
    {
        dest[i] = buffer.data[buffer.srcPtr++]
        buffer.srcPtr &=0x1F;    //buffer size of 31
    }
}

Replacing the if statement with a bit mask is much faster - especially on most microcontrollers, but it still looks less sleek than the original memory copy, doesn't it? This is where I thought the comma operator could "help" - by allowing me to write the same functionality like this:

void cbCopy(cbRef buffer, void * dest, size_t len)
{
    int i=0,endPtr;
    for(endPtr=(buffer.srcPtr+len)&0x1F;srcPtr!=endPtr;buffer.srcPtr=buffer.srcPtr++,buffer.srcPtr&0x1F)
    {
        dest[i++] = buffer.data[buffer.srcPtr]
    }
}

If you squint your eyes a bit and ignore most things that are happening in the for loop, this does indeed look sleek: there's only a single line inside the for loop and it looks like a memory copy. What's happening in the rest of it?

  • I changed the for loop from one that loops on from 0 to len to one that calculates an endPtr and then loops until the srcPtr is equal to it. This allows me to rewrite the for loop:
  • The initializer generates the endPtr.
  • The condition for terminating the loop is that the srcPtr is equal to the endPtr - i.e., we've copied all of the data we were asked to.

The increment is where the magic is. It (theoretically) uses the comma operator to join together two expressions: the first to increment the source pointer and the second to perform a bitmask on the pointer to wrap it back to 0 when it exceeds the length of the buffer. The comma operator should discard the result of the first increment and then return the value of the source pointer after it's been masked. 

I love being clever as much as the next guy and while I can admire clever, sleek pieces of code this just isn't one them. I can't recommend that you write code this way solely for the sake of using the comma operator for two three reasons:

  1. The comma operator is simply much less readable than the alternatives. Most people will be immediately confused by it and will have to fire up Google to see what's going on. This distraction makes it harder to read your code.
  2. It's rarely (if ever) necessary. There's always some combination of other, more readable statements that will give you the same effect.
  3. This code won't even work the way I expected it to: due to operator precedence, the 'b++' is evaluated first and the value returned, and the result of the mask is ignored.

My 'inventive' and 'clever' use of the comma operator changed a fairly straightforwad loop into a confusing mess that doesn't even fit into this article (on my browser anyhow) and wouldn't work if I ever coded it that way. Luckily, I took one look at that monstrosity and decided it wasn't worth it. Judging from a number of comments I've seen while researching this article, that seems to be the general consensus agreed to by most people (including the drafters of the MISRA C standard, which simply forbids the use of the operator). 

Still, I don't feel right having that be the final word. Yes, in this circumstance (and very likely, in most circumstances) using the comma operator produced code that was less readable than the alternative. That doesn't mean it's useless - it only means that we haven't found the appropriate use for it yet. In fact, you'll find a few practical, non-academic examples where the comma operator produces cleaner, more readable code. The answers to this Stackoverflow question have a few good examples:

So don't let the general opinion against the comma operator sway you. As with most things, programming skill improves when the programmer is challenged - regardless of whether the programmer passes the challenge or not! In this case, the challenge was to rewrite a memory copy loop using the comma operator (and maintain readability). I'd argue I failed this particular challenge but in the process improved my skills.  So I encourage you to challenge yourself: use the comma operator somewhere - anywhere! If it works out, great! If not, you'l have learned something - also great!  

UPDATE

Originally, I created an imaginary example using the comma operator:

a=b++,b>>2;

With the assumption that 'b++' would be executed and the result ignored and the outcome of 'b>>2' would be assigned to 'a' I thought the equivalent code to the above would be this:

b++;
a=b>>2;

This turned out not to be the case, but thankfully Old Wolf corrected me here and on reddit. Due to operator precedence rules in C he claimed, the equivalent code would actually be this:

a = b++;
b >> 2;

This would not have the intended effect at all. 

Now, I immediately believed Old Wolf because he said I was wrong - if there's anyone I don't trust it's me. However, sometimes I am in fact wrong about being wrong, so to determine which kind of wrong I was this time, I wrote this program (comma.c) to test the matter:

#include <stdio.h>
int main(void)
{
    int a=0;
    int b=7;
    a=b++,b>>2;
    printf("a is %d\r\n",a);
    printf("b is %d\r\n",b);
    return 0;
}

If I were correct I would expect a to be 2 and b to be 8. If Old Wolf were right, a will be 7 and b would be 8. So, which is it?

C:\Users\Stephen\Documents>gcc comma.c
C:\Users\Stephen\Documents>a.exe
a is 7
b is 8

I knew I should never have trusted me. My hat's off to Old Wolf. Apparently, an Old Wolf can teach a dog like me new tricks!

It's also worth noting that because of this error on my part, my circular buffer code wouldn't work correctly either. Luckily, that abomination is mainly meant to show that if you try to be too clever when writing code you'll end up making a mess of everything. Given that I tried to be clever and ended up making a mess out of everything, I think this article stands as sufficient proof by demonstration that you should stick with simplicity and clarity over clever tricks.


Previous post by Stephen Friederichs:
   Coding Step 2 - Source Control
Next post by Stephen Friederichs:
   Coding Step 3 - High-Level Requirements


Comments:

[ - ]
Comment by Old WolfJuly 23, 2015
a=b++,b>>2; is actually equivalent to a=b++; b>>2; not what you said. The comma operator has the lowest precedence.
[ - ]
Comment by SFriederichsJuly 23, 2015
I believe you. Just another reason this operator is confusing.
[ - ]
Comment by Old WolfJuly 23, 2015
This code causes undefined behaviour: buffer.srcPtr=buffer.srcPtr++,buffer.srcPtr&0x1F

it's equivalent to:

buffer.srcPtr = buffer.srcPtr++;
buffer.srcPtr & 0x1F;

The first line has the form i = i++ which is a well-known issue, and the second line has no effect.

Perhaps you meant buffer.srcPtr=(buffer.srcPtr++,buffer.srcPtr&0x1F)
[ - ]
Comment by Old WolfJuly 23, 2015
Further to my previous post - this would be much more clearly written as:

buffer.srcPtr = (buffer.srcPtr + 1) & 0x1F

without having to resort to shenanigans.
[ - ]
Comment by SFriederichsJuly 23, 2015
I protest - shenanigans is the whole POINT of this article.
[ - ]
Comment by Ricky58July 24, 2015
You said "That size is a power of 2 minus one"

No, the size is a power of 2, the mask is a power of 2 minus one.

Not directly related to the article, I know, but should be mentioned to eliminate confusion amongst the noobs.
[ - ]
Comment by SFriederichsJuly 24, 2015
The size of the buffer will actually be equal to a power of 2 minus one due to the way I planned to handle the empty/full confusion inherent in a circular buffer. You lose an element out of your buffer when you define a buffer to be 'full' when the tail pointer + 1 is equal to the head pointer.

For example, if the array holding the data is 32 bytes (with indexes of 0:31) and I define an empty buffer as head pointer == tail pointer, then initially head and tail pointers are at 0. When I add a byte, the tail pointer goes to 1, then 2, etc. all the way up to 31 (at which point the buffer holds 31 bytes). I cannot add another byte because if I do, the tail pointer will wrap around to 0 again and the head pointer will be equal to the tail pointer - I've defined this to mean 'empty' not 'full'. Thus, the buffer will only hold 31 elements even though the array is 32 elements long.
[ - ]
Comment by mathsengOctober 19, 2015
Maybe parenthesis will get you what you are seeking:
a=(b++,b>>2);
a is 2
b is 8


Breaking conventions of the use of macro, comma operator and ternary operators, here is a reworked buffer pop routine, modified from CBuf.h by Dave Hylands. Code done this way for a constrained embedded micro implementation.
(Requires that buffer size be a power of 2, enforced by macros defining the circular buffer.)

#define CBUF_Pop_( cbuf ) (cbuf.m_entry)[ cbuf.m_getIdx++ & (CBUF_Size(cbuf) - 1 )]
#define CBUF_Pop( cbuf, elem ) (CBUF_IsEmpty( cbuf ) ? NO_DATA : ((elem = CBUF_Pop_( cbuf )), RD_OK) )

// non-blocking read (extract)
uint32_t cbRead(uint8_t *buffer, uint32_t length)
{
uint8_t *iter = buffer;
uint8_t *end = iter + length;

while ((iter != end) && CBUF_Pop(rxQ, *iter))
++iter;

return (iter - buffer);
}

To post reply to a comment, click on the 'reply' button attached to each comment. To post a new comment (not a reply to a comment) check out the 'Write a Comment' tab at the top of the comments.

Registering will allow you to participate to the forums on ALL the related sites and give you access to all pdf downloads.

Sign up
or Sign in