On 2/22/14 7:18 AM, Kyle Huey wrote:
If you needed another reason to follow the style guide:
https://www.imperialviolet.org/2014/02/22/applebug.html
I don't really disagree with bracing being a good idea, but I'll be
contrarian here. Mandatory bracing probably wouldn't have helped; since
you could still easily end up with:
if ((err = SSLHashSHA1.update(&hashCtx, &foo)) != 0) {
goto error;
}
if ((err = SSLHashSHA1.update(&hashCtx, &bar)) != 0) {
goto error;
}
goto error;
if ((err = SSLHashSHA1.update(&hashCtx, &baz)) != 0) {
goto error;
}
There may even be an argument against bracing, in this specific case,
since it makes the repeated line slightly less visually obvious. I think
a remark about this was made in the recent style guide discussion: when
reviewing code, one often starts off by scanning for abnormal patterns
(hence of value of having consistent style, so "abnormal" isn't
conditional on which file you're in). But I don't really care. I think
the correctness-impact is usually exceedingly rare/minor, and the
ensuing style flamewars distract focus from better solutions.
For example, _both_ the Apple bug and my example above would have been
flagged by tools that warn about incorrect/misleading indentation.
Unsurprisingly, gps proposed doing this in the recent style guide thread
around automatic brace-insertion (and the risks thereto).
Dead/unreachable code analysis should have caught both as well, even
with "correct" indentation.
But really, the best way to fix this would be to use a macro:
err = SSLHashSHA1.update(&hashCtx, &foo);
SSL_ENSURE_SUCCESS(err, err);
err = SSLHashSHA1.update(&hashCtx, &bar);
SSL_ENSURE_SUCCESS(err, err);
err = SSLHashSHA1.update(&hashCtx, &baz);
SSL_ENSURE_SUCCESS(err, err);
Ok, maybe I'm not being entirely serious. :P
Justin
_______________________________________________
dev-platform mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-platform
