1. 12
  1.  

  2. 7

    More than the goto-issue, I believe implicit scoping is the real issue here. This is an excellent example of why it is a good idea to explicitly show the scope of if-else branching and similar constructs. In this case, adding braces after if(…) makes it very clear what scope that second goto belongs to.

    1. 4

      I doubt this. At least to me.

      if (ssl whatnot)
          goto fail;
          goto fail;
      if (ssl someother)
          goto fail;
      

      That looks obviously wrong to me because it’s indented without braces. I’d look closer. The hypothetical braced version

      if (ssl whatnot) {
          goto fail;
      }
      goto fail;
      if (ssl someother) {
          goto fail;
      }
      

      That looks less wrong to me. All the code is in the right place. Yeah, there’s a goto in the middle of the function, but it requires actually reading the statement and recognizing it as a goto. The first example doesn’t require any further processing of the statement type. I think the claim that the goto is easy to spot out of place is only because you know it’s a goto and it’s out of place. The original authors and reviewers were not asked “find the goto that’s out of place”. You have the benefit of a lot of hindsight.

      1. 3

        The point is that the code was probably originally

        if (ssl whatnot)
            goto fail;
        if (ssl someother)
            goto fail;
        

        and someone added another goto to the first branch. If it had originally been

        if (ssl whatnot) {
            goto fail;
        }
        if (ssl someother) {
            goto fail;
        }
        

        then the same line duplication would result in still-correct code:

        if (ssl whatnot) {
            goto fail;
            goto fail;
        }
        if (ssl someother) {
            goto fail;
        }
        
        1. 3

          Unless the added line went somewhere else:

          if (ssl whatnot) {
              goto fail;
          }
          goto fail;
          if (ssl someother) {
              goto fail;
          }
          

          It’s tempting to believe this would have failed in just the right way for our preferred coding style to prevent it, but that’s not guaranteed. It would be more impressive to find an positive example of a duplicated goto in real code that was rendered harmless by braces.

          1. 2

            It would be more impressive to find an positive example of a duplicated goto in real code that was rendered harmless by braces.

            Yes, because in that case it would not surface as a bug.

      2. 1

        This needs to be said more often, louder, and with more conviction. Brace-laziness has caused more errors than any other simple mistake I’ve seen.

      3. 2

        gotos are fine, but this serves as an “I told you so moment” for people who like:

        if (x) {
            goto fail;
        }
        

        over

        if (x)
            goto fail;
        
        1. 1

          Or people who like Python indentation-based syntax.

          1. 1

            How would that be an issue? If Python hypothetically had gotos, then

            if something:
                goto fail
                goto fail
            

            Would still work properly because the indentation level would be right.

            1. 2

              I mean that this serves as an “I told you so” moment for people who like Python indentation-based syntax, precisely because it would work properly — you can’t have the kind of misleading indentation in Python that was the problem here.

              1. 1

                I believe that is the point, it’s not an issue if things work like Python instead of C, that it serves as an “I told you so” for those who would equate indentation with block structure.

            2. 1

              Personally I like

              if (x) goto fail;
              

              and

              if (x) {
                  goto fail;
              }
              

              :-)

              (Not only does this style make it impossible for the programmer to forget to fix the presence/absence of braces when switching from one variant to the other. I just realised it also makes it impossible for the computer to mis-merge such code, if indeed that hypothesis is correct about how this duplicate goto fail happened. I.e. with this style such code must either merge correctly or cause a merge conflict.)

            3. 1

              Perhaps LLVM/Clang Static Analyser should produce warnings for unconditional use of goto?

              1. 1

                The unconditional use of goto in this case turned most of the function into unreachable code. Most linters will warn on unreachable statements, and clang has an option to do that, too; it’s just turned off by default for some reason.