5 ms·

Hilarious, yes. Except this: "The followers advice is dropping the curly brackets for these one-line ifs to make them all happy." Did I just read a code review

Hilarious, yes. Except this: "The followers advice is dropping the curly brackets for these one-line ifs to make them all happy." Did I just read a code review advocating for #gotofail, just to avoid a minor formatting nitpick?

cyphar 10y ago
It's also the Linux kernel style.
- Piskvorrr 10y ago
  Popularity doesn't make a bad idea any better.
  - asveikau 10y ago
    Sure, but you know what does make this idea better? The fact that it's pretty much the most sane way to consistently handle errors in a complicated C program. In particular if you have multiple paths out of your function and it also makes memory allocations, that's an overcomplicated amount of free() calls you need to take care of, remember are there, and maintain as you add/remove allocations and failure paths. goto lets you keep it simple by only maintaining a single block of free()s at your function's single exit path. Granted it seems this file is C++ and therefore the "going full RAII" option exists and could avoid goto in favor of exceptions or early returns... But if your code is C style that is a tough conversion to make, and not everybody buys into it equally.
    - Sean1708 10y ago
      But you could also do all of that without omitting curly brackets.
    - Piskvorrr 10y ago
      True, insightful, but completely beside the point - which was "bracketless conditionals open up an avenue for coding errors." The fail in #gotofail wasn't in using GOTO - it was a coding error, where a misindented line after a bracketless condition was always executed, when it should have been only executed inside the condition.
      - cyphar 10y ago
        You're conveniently not mentioning that this would cause very similar issues: if (some_cond) { goto fail; } goto fail; if (some_cond) { goto fail; } if (some_cond) { goto fail; } if (some_cond) { goto fail; } Is it more obvious? Sure. But the real issue is that they weren't testing all of the important failure cases, not that they weren't using one code style over another. Coding style is not going to help you if one of those some_cond values is incorrectly written...
  - digi_owl 10y ago
    When it shows up in the kernel it is likely to be deep in a hairy if cluster inside some driver code. Thus if you get a failure of some sort it is quite likely that it is best to just drop everything and get out, as the whole edifice is about to come down anyways.
    - Piskvorrr 10y ago
      if (something) { blah(); goto fail; } ^^^ that part works if (something) blah(); goto fail; ^^^ that part is written in a misleading way and doesn't do what it seems to do on first glance. It is unfortunate that the problematic line contained a jump, because "#gotofail" seems to convey "GOTO considered harmful", while it's something completely unrelated. Ironic, really, given the actual meaning of "human looks at a block of text and misparses it."
      - cyphar 10y ago
        You're conveniently not mentioning that this would cause very similar issues: if (some_cond) { goto fail; } goto fail; if (some_cond) { goto fail; } if (some_cond) { goto fail; } if (some_cond) { goto fail; } Is it more obvious? Sure. But the real issue is that they weren't testing all of the important failure cases, not that they weren't using one code style over another. Coding style is not going to help you if one of those some_cond values is incorrectly written...
        Piskvorrr 10y ago
        "More obvious" is precisely my point: the misindentation in your example jumps out at anyone familiar with the syntax. This is not an universal cure, and won't help you with other types of semantic errors: nothing is going to help you if you try to avoid thinking and rely solely on your tools; that is the essence of cargo-cult programming. OTOH, this tool will help you avoid a common pitfall with potentially disastrous consequences. A high-visibility vest for coding, if you will: won't save you from slipping or your head from falling objects, but neither is its purpose. (Iterative bootstrapping is how we got from sharpened rocks to virtual machines, not by "meh, this minor improvement won't fix everything, so why bother.")
  - loeg 10y ago
    Popularity may suggest that it is not a bad idea.
    - Piskvorrr 10y ago
      May or may not. Given that popularity breeds more popularity, all it actually says is that it was popular enough, sometime in the past, to start a runaway process (I don't really want to start a flame war of "look at those popular yet fairly horrible things/languages/whatevers")
anarazel 10y ago
Compilers warn against misleading indentation these days, so I don't think that line of argument is all that strong these days.
omginternets 10y ago
What exactly is the risk here?
- stonemetal 10y ago
  Here? Not much but as a style choice it is what lead to an SSL bug in iOS. https://www.imperialviolet.org/2014/02/22/applebug.html https://www.imperialviolet.org/2014/02/22/applebug.html
- chris_wot 10y ago
  Reduced mooing.
mnarayan01 10y ago
I thought it was a (pretty funny) joke.
deleted 10y ago
[deleted]
Grue3 10y ago
Python has #gotofail built-in, and it's considered by fans to be an awesome feature of the language!
- Piskvorrr 10y ago
  That's not a bug, that's a feature! :D

cyphar 10y ago

It's also the Linux kernel style.

Piskvorrr 10y ago
Popularity doesn't make a bad idea any better.
- asveikau 10y ago
  Sure, but you know what does make this idea better? The fact that it's pretty much the most sane way to consistently handle errors in a complicated C program. In particular if you have multiple paths out of your function and it also makes memory allocations, that's an overcomplicated amount of free() calls you need to take care of, remember are there, and maintain as you add/remove allocations and failure paths. goto lets you keep it simple by only maintaining a single block of free()s at your function's single exit path. Granted it seems this file is C++ and therefore the "going full RAII" option exists and could avoid goto in favor of exceptions or early returns... But if your code is C style that is a tough conversion to make, and not everybody buys into it equally.
  - Sean1708 10y ago
    But you could also do all of that without omitting curly brackets.
  - Piskvorrr 10y ago
    True, insightful, but completely beside the point - which was "bracketless conditionals open up an avenue for coding errors." The fail in #gotofail wasn't in using GOTO - it was a coding error, where a misindented line after a bracketless condition was always executed, when it should have been only executed inside the condition.
    - cyphar 10y ago
      You're conveniently not mentioning that this would cause very similar issues: if (some_cond) { goto fail; } goto fail; if (some_cond) { goto fail; } if (some_cond) { goto fail; } if (some_cond) { goto fail; } Is it more obvious? Sure. But the real issue is that they weren't testing all of the important failure cases, not that they weren't using one code style over another. Coding style is not going to help you if one of those some_cond values is incorrectly written...
- digi_owl 10y ago
  When it shows up in the kernel it is likely to be deep in a hairy if cluster inside some driver code. Thus if you get a failure of some sort it is quite likely that it is best to just drop everything and get out, as the whole edifice is about to come down anyways.
  - Piskvorrr 10y ago
    if (something) { blah(); goto fail; } ^^^ that part works if (something) blah(); goto fail; ^^^ that part is written in a misleading way and doesn't do what it seems to do on first glance. It is unfortunate that the problematic line contained a jump, because "#gotofail" seems to convey "GOTO considered harmful", while it's something completely unrelated. Ironic, really, given the actual meaning of "human looks at a block of text and misparses it."
    - cyphar 10y ago
      You're conveniently not mentioning that this would cause very similar issues: if (some_cond) { goto fail; } goto fail; if (some_cond) { goto fail; } if (some_cond) { goto fail; } if (some_cond) { goto fail; } Is it more obvious? Sure. But the real issue is that they weren't testing all of the important failure cases, not that they weren't using one code style over another. Coding style is not going to help you if one of those some_cond values is incorrectly written...
      - Piskvorrr 10y ago
        "More obvious" is precisely my point: the misindentation in your example jumps out at anyone familiar with the syntax. This is not an universal cure, and won't help you with other types of semantic errors: nothing is going to help you if you try to avoid thinking and rely solely on your tools; that is the essence of cargo-cult programming. OTOH, this tool will help you avoid a common pitfall with potentially disastrous consequences. A high-visibility vest for coding, if you will: won't save you from slipping or your head from falling objects, but neither is its purpose. (Iterative bootstrapping is how we got from sharpened rocks to virtual machines, not by "meh, this minor improvement won't fix everything, so why bother.")
- loeg 10y ago
  Popularity may suggest that it is not a bad idea.
  - Piskvorrr 10y ago
    May or may not. Given that popularity breeds more popularity, all it actually says is that it was popular enough, sometime in the past, to start a runaway process (I don't really want to start a flame war of "look at those popular yet fairly horrible things/languages/whatevers")

anarazel 10y ago

Compilers warn against misleading indentation these days, so I don't think that line of argument is all that strong these days.

omginternets 10y ago

What exactly is the risk here?

stonemetal 10y ago
Here? Not much but as a style choice it is what lead to an SSL bug in iOS. https://www.imperialviolet.org/2014/02/22/applebug.html https://www.imperialviolet.org/2014/02/22/applebug.html
chris_wot 10y ago
Reduced mooing.

mnarayan01 10y ago

I thought it was a (pretty funny) joke.

deleted 10y ago

[deleted]

Grue3 10y ago

Python has #gotofail built-in, and it's considered by fans to be an awesome feature of the language!

Piskvorrr 10y ago
That's not a bug, that's a feature! :D