Hacker News Reader

Comment by voltagex_ | Hacker News Reader

Lines like that cause conditions to be missed (e.g. Apple's goto fail bug)

    if (rc < 0)
    {
      return rc;
    }

always IMO.

View on HN

Only if they're not on the same line though. When you have

    // ...
    if (rc < 0)
      return rc;
    // ...

It's easy to confuse the indentation for a block and end up with something like the apple bug.

But when you don't break the line there's no confusion:

    // ...
    if (rc < 0) return c;
    // ...

I don't see how this is any less confusing.

  if (rc < 0) return rc;
    goto fail;

is no less confusing to me than

  if (rc < 0)
    return rc;
    goto fail;

Both can easily be misunderstood when you're quickly scanning through a bunch of code. Screen space isn't at such a premium that you can't afford a couple extra lines in a conditional statement.

How would you end up with

    if (rc < 0) return rc;
      goto fail;

Why would you indent hat next line? Presumably because you were trying to add it to the existing if statement's non-existant block. But why would you do that if you don't see a block?

The problem with

    if (rc < 0)
      return rc;

is that we see the indentation and infer that there must be a block, so it's tempting to try to add to the block.

When you keep your entire conditional statement on one line, it looks, to someone scanning the code, like just any other line. It doesn't look like a block at all, in fact it looks more like a function call.

> Why would you indent hat next line? There are lots of reasons someone might indent the next line. Maybe they were in a rush. Maybe they were distracted by something else while they were working. Maybe they were tired. Maybe they were drunk.

Mistakes will happen. Ideally wrong code will look wrong. All of the following examples do the same thing. If you know the code is supposed to call Foo() and Bar() if bVar is true, which of these examples make it obvious that something is wrong?

  if (bVar) Foo();
    Bar();

  if (bVar)
    Foo();
    Bar();

  if (bVar)
  {
    Foo();
  }
    Bar();

  if (bVar)
  {
    Foo();
  }
  Bar();

The third, followed by the fourth, makes the mistake most obvious to me.

The one thing I've never seen mentioned in style guide is that maybe the same piece of code should take the style of whatever is fitting in its context?

For example, let's say there are 20 case where it's a single line return, then shouldn't be "if(rc < 0) return rc;" better. On the other hand, if the other condition in that block of code all need brackets, it just seems good sense for the single line condition to have bracket too.

It could be extending pretty much to all other syntactic sugar even in higher level language (map + lambda vs for loop?)

Yeah, the replies here are making me doubt myself - I probably just need to slow down when reading code

Interestingly, the author goes on to recommend

  if(rc < 0) goto fail;

as a countermeasure for that breed of bug.

gcc won't catch that kind of bug, but clang will definitely. So yeah, use static analysis tools.

I usually write stuff like this

  if((rtn = func(/*whatever*/)) < 0)
  {
    // some hint why things may fail
    goto barfo;
  }

Reply on news.ycombinator.com