Hacker News Reader

Comment by judemelancon | Hacker News Reader

I am hardly astonished that a 319-line function that opens by declaring x, xtmp, xtmp2, chain_ss, bad_chain, param, depth, i, ok, num, j, retry, cb, and sktmp variables had a bug.

Before someone provides the standard "submit a patch" retort, I'll note that the variable naming is in full compliance with https://www.openssl.org/about/codingstyle.txt even if the function length isn't. A quick sample of other files suggests the function length matches actual practice elsewhere, too.

View on HN

And the coding style doesn't exactly avoid errors as well.

"Do not unnecessarily use braces around a single statement:

    if (condition)
        action();

and

    if (condition)
        do_this();
    else
        do_that();

Didn't people learn from the goto fail bug ? http://embeddedgurus.com/barr-code/2014/03/apples-gotofail-s...

Personally, I say if a statement continues on a different line, then you should use braces

    //okay
    if (condition) return foo;

    //not okay
    if (condition)
        return foo;

    //not okay
    if (condition) do_foo();
    else do_bar();

In the second case, the else can be considered a continuation. In the first example, there's little chance of confusion or the introduction of an error, in the second and third, that is not necessarily the case.

If it doesn't look/fit well on one line, break it up with braces.

What about:

if((somevar != checkvar((byte)othervar)) & (i != 3)) somefunc();

someotherfunc();

A bit exaggerating, I agree, but not far off from some real-world examples and quite confusing.

Nice, in that case, I might advocate for either somefunc or a function in front of it taking somevar, othervar and i as parameters with early return statements before falling through to the work.. returning a boolean for if they passed through...

I've seen far, far, far worse...

> Do not unnecessarily use braces around a single statement

If you consider all braces to be necessary, then that's a truism and you can safely ignore it. "I didn't unnecessarily use them: we always use them because that's the safe thing to do."

Maybe the coding standard was written by someone who was used to auto-indentation making this sort of mistake immediately obvious.

Reading your comment I initially assumed {i, j} were iterators, in which case they would be fine by me. Nope.

You seriously have trouble reading "i", "j", "param" and "num"? Hell, "ok", "depth" and "retry" are already in your English-language dictionary!

I'll grant that having variables named with "tmp" is confusing out of context, I guess. But if you're trying to start a Java-style war over this stuff, just recognize that most of the world has moved on and views names like those as perfectly fine when used within standard idioms.

I have trouble understanding what they're meant to signify, yes. I am a regular human with finite intellect; I admit it.

To actually comprehend this function requires storing those 14 names in short-term memory, reading through the over 300 lines of remaining code, filling in bits of the meaning of those names as they become clear, and only then reading the code again with that mental map. That's the case where none of the 14 have slipped my mind by the time I get back around. That just seems like an awful lot of overhead to net something that could be as easy as reading names if they were better chosen.

Just as a demonstration, why don't you time how long it takes you to figure out what j actually is and then report back?

I don't know the codebase, but if for example, there's a project wide standard or convention for what the variable j means, then it could be OK. Or at least not as bad as it first appears. Not all code has to be written for an assumed stranger on the street trying to sight-read it.

This is OpenSSL we're talking aboot here.

The use of shorthand names like that is a strong indication that the variables don't need to be live for ~319 lines. If you reduce the live range of your variables, you reduce the complexity of your function. Less complex functions are less likely to have bugs and are easier to diagnose when they do.

Good points, but if you are able to do that, doesn't it suggest the function could easily be split into multiple, more specific functions?

Yes, of course, it's just the first step. In fact you'd probably need to introduce a couple more variables. For example, i gets defined three times:

  i = sk_X509_num(ctx->chain);
  i = check_trust(ctx);
  i = X509_chain_check_suiteb(&ctx->error_depth, NULL, ctx->chain, ctx->param->flags);

What a mess. I would probably start by moving towards a module / class for all the functions that take either an X509_STORE_CTX *ctx pointer or something accessed through ctx.

Are you kidding me? There is no possible universe in which 'param' is an acceptable name for an argument.

What about this?

  class Function<T> {
    public apply(T param);
  }

OK. One possible universe.

Technically speaking, that's a parameter, not an argument.

A three hundred nineteen line function though with all those variables is more than a bit big. Modern compilers are rather good at inlining code, so it's doubtful there will be very much, if any effect on codesize there. The multitude of temporary variables also suggests that there are probably portions of that function that need factored out. OpenSSL suffers from a severe thousand paper cuts problem. While its warts aren't that bad when viewed individually, the sheer multitude shows a severe lack of organization of the project from top to bottom.

Reply on news.ycombinator.com