Brush up code style?

[irwir]

The suggestion came up in the issue #10448, but may have wider scope - the whole library code and example programs.

1. Assignments in conditional expressions.
   This idiom remains in the library from the days of PolarSSL.
   It mostly helped to get efficient object code with relatively primitive compilers, but such conditions are harder to read and are known to be error-provoking.
   Extra set of parentheses usually suppresses compiler's warning, though static analysis still might flag it.

2. Declaration of temporary variables where required, not at the function entrance (use C99 standard's advantages)
   Older programming languages required declaration to precede any code; and the library adheres to this obsolete and inconvenient pattern.
   Possible negative effects (and potential warnings) are: uninitialized variable, initialization with an unrelated value/assigned value was never used.
   More recent developments allow declarations anywhere. This makes code closer to RAII paradigm and easier to read because variable declaration would be nearby, not a few lines away. Then, lengthy list of declarations at the beginning of a function would get shorter.

The original code was:

mbedtls/library/ssl_tls13_generic.c

Line 219 in d4b6bc7

int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;

mbedtls/library/ssl_tls13_generic.c

Lines 303 to 309 in d4b6bc7

	if ((ret = mbedtls_pk_verify_new(sig_alg,
	&ssl->session_negotiate->peer_cert->pk,
	md_alg, verify_hash, verify_hash_len,
	p, signature_len)) == 0) {
	return 0;
	}
	MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_pk_verify_new", ret);

Taking the above points into account, the code could be rewritten as:

    int ret = mbedtls_pk_verify_new(sig_alg,
                      &ssl->session_negotiate->peer_cert->pk,
                      md_alg, verify_hash, verify_hash_len,
                      p, signature_len);
    if (ret == 0) {
        return ret;
    }
    MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_pk_verify_new", ret);

[ronald-cron-arm]

Hi, thanks for your proposal.

Regarding 2, our coding standards recommend keeping the scope of variables as small as possible, see the "Variable initialization" section in Mbed TLS coding standards. This is not something that can easily be enforced with tooling, so I’d say there’s limited room for improvement here.

Regarding 1, we neither encourage nor discourage assignments in conditional expressions.
We mitigate the “error-prone” aspect with the -Wparentheses compiler warning, so I don’t see a strong case for further action on that front either.

Regarding the code change proposed in #10448, I think it's a good proposal and is compliant with our coding standards.

[irwir]

Regarding 2, ...
This is not something that can easily be enforced with tooling, so I’d say there’s limited room for improvement here.

Is this really limited? Previous code formatting rules were peculiar (as defined by one of the maintainers), yet enforced quite successfully without any automation.

Even recent code additions keep declaring variables after function's opening brace, not where it would be used. Either the recommendation about smallest scope is misunderstood - or ignored, because it would deviate from historical style in the existing code base.

Regarding 1, we neither encourage nor discourage assignments in conditional expressions.

Hence the style in this aspect also remains historical.