9 ms·
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 v
by judemelancon 11y ago
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 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.
- ajross 11y agoYou 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.
- ectoplasm 11y agoThe 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.
- jfoster 11y agoGood points, but if you are able to do that, doesn't it suggest the function could easily be split into multiple, more specific functions?
- ectoplasm 11y agoYes, 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.
- judemelancon 11y agoI 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?
- ams6110 11y agoI 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.
- alricb 11y agoThis is OpenSSL we're talking aboot here.
- d_theorist 11y agoAre you kidding me? There is no possible universe in which 'param' is an acceptable name for an argument.
- mayanksinghal 11y agoWhat about this? class Function<T> { public apply(T param); }
- d_theorist 11y agoOK. One possible universe.
- ectoplasm 11y agoTechnically speaking, that's a parameter, not an argument.
- Sanddancer 11y agoA 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.
- sliverstorm 11y agoReading your comment I initially assumed {i, j} were iterators, in which case they would be fine by me. Nope.
- rgj 11y agoAnd 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-ssl-security-bug-was-easily-preventable/ http://embeddedgurus.com/barr-code/2014/03/apples-gotofail-s...
- kstrauser 11y ago> 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."
- sjolsen 11y agoMaybe the coding standard was written by someone who was used to auto-indentation making this sort of mistake immediately obvious.
- tracker1 11y agoPersonally, 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.
- breakingcups 11y agoWhat 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.
- tracker1 11y ago