3 ms·

It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. You don't wan

It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. You don't want your own "goto fail" do you? ;) [1] I think you did a great job. [1] https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/ https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-got...

chjj 5y ago
> It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. > You don't want your own "goto fail" do you? ;) [1] Ah, that was my style 7+ years ago. Then I started regularly contributing to an open source project which did not add curly braces on one-liners. I wanted to match the style of the project despite it feeling unnatural to me. Within a few weeks, my style was changed forever (for better or for worse). > I think you did a great job. Thank you.
kazinator 5y ago
Won't necessarily work: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } { goto fail; /* MISTAKE! THIS BLOCK SHOULD NOT BE HERE */ } Copy and paste bugs can play out in any number of ways. Newer GCC will catch the goto fail by noting that the indentation is wrong. $ gcc-11 -Wall gotofail.c gotofail.c: In function ‘main’: gotofail.c:5:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] 5 | if (argc > 42) | ^~ gotofail.c:7:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ 7 | goto fail; | ^~~~ $ cat gotofail.c #include <stdlib.h> int main(int argc, char **argv) { if (argc > 42) goto fail; goto fail; return 0; fail: return EXIT_FAILURE; } Of course, it could be that the indentation is not wrong. If somneone runs the code through some automatic formatting, the problem will not then be diagnosable that way. Sadly, if I fix the indentation then: $ gcc-11 -Wall -Wunreachable-code -O3 gotofail.c $ # silence even though the return 0; is not reachable. Still, I think I'm going to stick with "brace the else part if the then part is braced, and vice versa*.

chjj 5y ago

> It looks super clean to me, the only exception being for the love of all that's good and holy always brace your one-line if statements and loops. > You don't want your own "goto fail" do you? ;) [1] Ah, that was my style 7+ years ago. Then I started regularly contributing to an open source project which did not add curly braces on one-liners. I wanted to match the style of the project despite it feeling unnatural to me. Within a few weeks, my style was changed forever (for better or for worse). > I think you did a great job. Thank you.

kazinator 5y ago

Won't necessarily work: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } { goto fail; /* MISTAKE! THIS BLOCK SHOULD NOT BE HERE */ } Copy and paste bugs can play out in any number of ways. Newer GCC will catch the goto fail by noting that the indentation is wrong. $ gcc-11 -Wall gotofail.c gotofail.c: In function ‘main’: gotofail.c:5:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] 5 | if (argc > 42) | ^~ gotofail.c:7:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ 7 | goto fail; | ^~~~ $ cat gotofail.c #include <stdlib.h> int main(int argc, char **argv) { if (argc > 42) goto fail; goto fail; return 0; fail: return EXIT_FAILURE; } Of course, it could be that the indentation is not wrong. If somneone runs the code through some automatic formatting, the problem will not then be diagnosable that way. Sadly, if I fix the indentation then: $ gcc-11 -Wall -Wunreachable-code -O3 gotofail.c $ # silence even though the return 0; is not reachable. Still, I think I'm going to stick with "brace the else part if the then part is braced, and vice versa*.