Hacker News Reader

Comment by owyn | Hacker News Reader

On second thought, let's not go to Camelot. It is a silly place.

It's useful shorthand in a few specific cases, but I would argue that this sort of code is bad style and if I was in charge (which I am not) it would be in the "never do this" section.

That this edge case apparently took decades to find implies (to me) that the comma operator is not to be used in production code. I'll always choose a few extra lines of code over ambiguity, because someday other will people have to read my code when there's some kind of a problem with it, and I'd rather make the problems obvious, not subtle. :)

View on HN

The problem is that if you try to rewrite that example to avoid the comma operator, you either have to duplicate the update_thing(&foo) call, as in:

  update_thing(&foo);
  while (foo != 0) {
      /* loop body */

      update_thing(&foo);
  }

Unnecessary duplication like this is itself a potential source of bugs - it's all to easy to update one but not the other. The other alternative is to hack up the loop to exit in a strange place:

  while (1) {
      update_thing(&foo);
      if (foo == 0)
          break;

      /* loop body */
   }

This is arguably even worse - the actual loop termination condition is not where you expect to find it anymore. Personally, I find the formulation using the comma operator to be completely clear.

Note that the bug referenced here is more about the subtleties of bitfield type promotion in expressions, and the interplay of bitfields with operators that evaluate to the type of one of their arguments, than it is about the comma operator. You can show the same bug using the assignment operator instead of the comma operator.

If you're going consign anything involved here to the "never do this" section, I'd start with any use of bitfields, and maybe also include mixing unsigned and signed types in expressions without explicit conversion.

I completely agree with everything you've said except for "any use of bitfields." The comma operator is ok in this circumstance, mixing unsigned and signed is a big no-no, but what have you got against bitfields? They're very useful when it comes to pulling sub-byte fields out of network packets.

They're very useful when it comes to pulling sub-byte fields out of network packets.

That is, if you don't mind tying your code to a particular platform and compiler. The alignment and packing of bitfields is implementation dependent: http://stackoverflow.com/questions/1490092/c-c-force-bit-fie...

For what it's worth, I use bitfields on occasion, but only to save memory and get better warnings when storing range-limited values. It's nice to have the compiler warn about a comparison always being true or false due to the limited range of a type. The performance of bitfields is probably worse than just tossing all my boolean values into an int32_t or int8_t.

Aha, thank you. As most of my C code has been written for a single compiler and a single platform I was less aware of the pitfalls than I should have been. I will know now to be extra vigilant if I ever have to port that code.

They're not any use for pulling sub-byte fields out of network packets in portable code, because everything about the precise layout of bitfields is entirely implementation-defined. Portable code has to use masking-and-shifting.

The only portable uses of bitfields are to potentially save a little memory, and to get "modulo-power-of-2" behaviour. Bitfields are a classic "not as useful as they first appear" feature.

We're coming in the dangerous area of personal preferences, but with the decades of writing and also maintaining code, I certainly prefer "exit in a strange place" variant, as it clearly better allows the code to "grow" with less differences between revisions, like adding a few more lines between update_thing and if. And if the revisions are not expected then how is written is in my opinion realy irrelevant. (Disclaimer: I won't discuss this subject more here.)

  do {
      update_thing(&foo);
  } while (foo != 0);

This works for the example as typed, but not if there is a body to the original loop.

A little ugly but you can get it to work without duplicate code by:

  bool second_loop = false;
  do
  {
  if (second_loop) {...}
  update_thing(&foo);
  second_loop = true;
  } while (foo != 0);

The real question is which is more clear when some else looks at the code. I suspect even if it's shorter and faster while (foo,bar) is less clear to most people than the above code.

Loop-and-a-half is a very common and useful pattern, I don't see anything wrong with the second code snippet.

Well, the problem here is (assuming I'm understanding everything correctly) pretty specifically the combination of the comma operator and a bitfield. Of those two, I'd say the latter, even if superficially more intuitive, is probably the more obscure and advisable to avoid.

The comma operator, while not all that common, can be pretty useful in common situations, and can easily achieve both clarity and conciseness. For example, to iterate over a singly-linked list while keeping a pointer to the previous item:

  for (prev = NULL, this = first; this != NULL; prev = this, this = this->next) {
    ...
  }

I think this is a perfectly reasonable way to do things. Disallowing the comma operator due to an underspecified interaction with bitfields would be throwing out the baby with the bathwater, in my opinion.

Reply on news.ycombinator.com