Hacker News Reader

Comment by zh3 | Hacker News Reader

Funnily enough just spent a day tracking down a weird problem in an embedded system - some event timestamps were getting corrupted in a weird way. The pattern wasn't obvious until in desperation I dumped them in hex and found the second MSB was toggling between 0 and 1 (whereas it should have been been part of a count sequence). That told me exactly where to look - where the count was re-assembled from four bytes and I found this upstream (paraphrased):-

    uint8_t tx_data[64];

    ....
    tx_data[43] = utime>>24;
    tx_data[44] = utime>16;
    tx_data[45] = utime>>8;
    tx_data[46] = utime;

View on HN

This is one case where lining up values (and surrounding operators with spaces) can be good practice:

    tx_data[43] = utime >> 24;
    tx_data[44] = utime >> 16;
    tx_data[45] = utime >>  8;
    tx_data[46] = utime >>  0;

Or even:

    tx_data[43] = utime >> (3 * 8);
    tx_data[44] = utime >> (2 * 8);
    tx_data[45] = utime >> (1 * 8);
    tx_data[46] = utime >> (0 * 8);

Yep. Bugs like this just pop out at you this way.

Having spent a lot of time digging into C code and doing microcontroller programming professionally, I have learned a certain disdain for the practice of using minimal whitespace in expressions, and the related practice of minimising LOC using C's very dense expressions syntax(unary dec/inc operators, the fact that assignment is itself an expression, etc).

The dense expressions syntax is pretty much a holdover from the time C was invented by Dennis Ritchie for the purpose of porting Unix; when it had to be typed on a painfully slow electromechanical teletype. This is also why most Unix commands are 2-4 characters long.

But it serves very little meaningful purpose today. It's important to remember that you might be able to turn 3 lines of code into 1, but it'll still be the same amount of machine code. And it will probably be harder to read, harder to change, and harder to reason about. And it can be a lot easier to miss some edgecase or even a typo like in this case. I have seen such a silly amount of off-by-one errors in C code due to some subtle misuse of unary increment/decrement that I stopped using those operators altogether. They don't improve your software in any meaningful way.

I've done the >> 0 thing to make it very clear what's going on, but I hadn't considered the * 8 construction. That's actually easier to comprehend at a glance because the numbers become less "magic". (8 and 16 are obvious to me, but 24 always has me second-guessing myself somewhere in the back of my mind)

I use octal for this.

  x << (3 * 8);

vs.

  x << 030;

       tx_data[44] = utime>16;

Good catch!

I wonder if this would have been flagged with -Wall?

Nope. There's no warning for bool->int conversion: https://stackoverflow.com/questions/28716391/gcc-forbid-impl...

Somewhat oddly even in C++, where bool is a distinct type, g++ doesn't have any warning for implicit bool to int conversion, although clang's clang-tidy "linter" tool does.

Nope, code was clean with -Wall on arm-9 compiler (i.e. gcc).

Interesting thought now you say that (no warning) - only found it because of seeing the pattern (apropos the article) and from that having a good idea what was causing it (knowing the int was assembled a byte at a time).

If I had a criticism of modern compilers, it's the blizzard of uninteresting warnings ("strncmp takes const char star, did you really mean to pass it unsigned char star") that make people want to not use -Wall.

All warnings are uninteresting until they're not.

> ("strncmp takes const char star, did you really mean to pass it unsigned char star")

This warning (with a different function) actually saved my bacon once, pointing me to a very obscure bug in the code.

My practice is to use -Wall and make sure that the code compiles without any warnings at all. Then I don't have a deluge of warnings to wade through.

Remember -Wall isn't all. -Wall -Wextra is the new -Wall.

I recommend asan too, where possible.

Oh, you're right. I need to update my build system.

Why do compilers do stupid things like this? Seriously don't get why -Wall isn't all warnings.

clang-tidy has an open issue to catch this. https://github.com/llvm/llvm-project/issues/56009

This is a good example

Why C's implicit conversions are trash. Why big endian is trash. Why you should do a reverse copy instead of stuff like this.

Reply on news.ycombinator.com