1602248 - Remove no-longer-protective "Always brace controlled statements" C++ style recommendation

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Lint and Formatting, task)

Description

[Kelsey Gilbert [:jgilbert]]

This came up in bug 1602143.

clang-format (and clang-tidy readability-misleading-indentation) protect us from dangling-else concerns. Here's the classic goto-fail example, if it were ./mach clang-formatted:

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail;                                                                                                                                                                                                                    err = sslRawVerify(/*...*/);                                                                                                             }                 

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1602248 - Remove "Always brace controlled statements" from C++ style.

clang-format (and clang-tidy readability-misleading-indentation) protect
us from dangling-else concerns.

Here's the classic goto-fail example, if it were ./mach clang-formatted:

void GotoFail() {
hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0) goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail;

err = sslRawVerify(/.../);
}

Comment 2

[Sylvestre Ledru [:Sylvestre]]

This isn't only about dangling-else but to bring consistency and common practices in our code base.

Comment 3

[(no longer active)]

As far as I'm aware, we don't run clang-tidy readability-misleading-indentation rewriters automatically on our code, so the assertion made in comment 0 about that tool plus clang-format protecting our code from the classic goto-fail example isn't really true. Furthermore, I don't really think there is broad support for the change you're suggesting here, Jeff. To give a concrete example, the SpiderMonkey team went to great lengths to convert their entire code to use braces around if/for/while statements (see bug 1488698).

I think this is very well into the territory of people's individual personal preferences, and the best historical date that we have here tells us this is one of the aspects of the old Mozilla coding style that was in fact quite popular. Without knowing whether that situation has indeed changed, I'm very hesitant towards changing here. If not because of any other reason, because I think you didn't need to argue this far out just for bug 1602143. :-) I'll make a comment on that bug.

For now, closing this as WONTFIX. Please reopen if you have evidence that shows this has gained broad support these days. Thanks!