Re: [Qemu-devel] [7234] Use a more natural order

[Jamie Lokier <jamie@shareable.org>]

Lennart Sorensen wrote:
> On Thu, Apr 23, 2009 at 10:31:05PM +0300, Blue Swirl wrote:
> > I don't think any code style document can cover all possible cases.
> > But another approach can be used: you could try to find a precedent
> > case where this style has been used in QEMU.
> 
> # grep -r '([0-9] =' .
> ./net.c:    if (0 == errno && '\0' == *last_char &&
> ./hw/gus.c:    if (0 == ((mode >> 4) & 1)) {
> ./hw/dma.c:            if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
> ./hw/sb16.c:        if (0 == s->needed_bytes) {
> ./hw/sb16.c:            if (0 == s->needed_bytes) {
> ./hw/sb16.c:        if (0 == s->dma_auto) {
> ./hw/sb16.c:        if (0 == s->dma_auto) {
> 
> That was just one quick search.  Looks like whoever wrote a bunch of
> the audio hardware emulation liked less buggy code.

No, it means they believe that way around makes them less likely to
write bugs.  They may be right for themselves.  It doesn't mean the
code is less buggy, and it doesn't mean someone else following the
same style would write less buggy code because of the style.

Same for the redundant parentheses around every comparison next to &&.
Really, there is no need: if (condition && condition) is so common
that there's no need to clarify the grouping.  Unlike say "x & 1 << 2"
where it's a good idea to add parentheses.

> Well to me software development is a kind of engineering and hence using
> anything but the safest practice that is at all practical makes no sense.

But what you call safest isn't safest for everyone.  A lot depends on
what people are familiar with and trained to spot deviations from.

> That means:
> Constants before variables in all comparisons.
> Braces are never optional for blocks.

I disagree with both, but especially the first.  You call it safe
practice.  I call it likely to cause more bugs because it's unfamiliar
compared with common practice.  And because it's the opposite to the
way we say it mathematically or in English.

It's opposite to the "Object Verb Subject" grammar that English
speakers think in: asking "is zero equal to x?"  is really weird
English, because that is asking a question _about zero_, which is not
right: zero is not meant to be something about which there is any question.
It's a constant!

That cognitive dissonance adds a translation step between thinking (in
English) and reading/writing, which means more chance of not seeing
other bugs - limited brainpower and all.

"if (0 < x)" is even worse: it's begging to be accidentally written
with ">".  More importantly, if it _is_ accidentally written with ">",
the mistake is less likely to be noticed than if it were written the
other way around.  But if you write "if (0 == x)" and "if (x > 0)"
just because of "=" C syntax, that's an inconsistent mess.

Do you also write things like "for (x = 0; NUMBER > x; x++)"?  No, of
course not.

Sure, it was ok practice back in the days when compilers would let you
write "if (x = 0)", but those days are long gone.  Now all compilers
warn, and you can ask them to make it an error.  A compiler check is
much safer than depending on writing style, especially a weird one
that's inconsistent with other writing in the same code, and
inconsistent with English thinking.

> The second one is especially hard to get some people to understand.

Accidentally mistaking the scope of a block is unlikely using proper
tools.  We're not using NOTEPAD.EXE any more.  Editors can indent code
automatically these days (last 20 years or so).  Again, using tools is
better than depending on writing style because it catches more mistakes.

Actually I'm sympathetic to people who prefer braces around every
block, and I happily follow such a style when requested.  It doesn't
look too painful or weird, unlike the "unnatural" comparison order. :-)
But I don't think it's necessary on projects where everyone uses other
equally good practices instead to catch the same errors.

-- Jamie