Re: [PATCH] docs/style: allow C99 mixed declarations

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 6/2/24 06:53, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>>> On Mon, Feb 05, 2024 at 12:18:19PM -0500, Stefan Hajnoczi wrote:
>>>> C99 mixed declarations support interleaving of local variable
>>>> declarations and code.
>>>>
>>>> The coding style "generally" forbids C99 mixed declarations with some
>>>> exceptions to the rule. This rule is not checked by checkpatch.pl and
>>>> naturally there are violations in the source tree.
>>>>
>>>> While contemplating adding another exception, I came to the conclusion
>>>> that the best location for declarations depends on context. Let the
>>>> programmer declare variables where it is best for legibility. Don't try
>>>> to define all possible scenarios/exceptions.
> ...
>
>>> Even if the compiler does reliably warn, I think the code pattern
>>> remains misleading to contributors, as the flow control flaw is
>>> very non-obvious.
>> 
>> Yup.  Strong dislike.
>> 
>>> Rather than accept the status quo and remove the coding guideline,
>>> I think we should strengthen the guidelines, such that it is
>>> explicitly forbidden in any method that uses 'goto'. Personally
>>> I'd go all the way to -Werror=declaration-after-statement, as
>> 
>> I support this.
>> 
>>> while C99 mixed decl is appealing,
>> 
>> Not to me.
>> 
>> I much prefer declarations and statements to be visually distinct.
>> Putting declarations first and separating from statements them with a
>> blank line accomplishes that.  Less necessary in languages where
>> declarations are syntactically obvious.
>
> But we already implicitly suggest C99, see commit ae7c80a7bd
> ("error: New macro ERRP_GUARD()"):
>
>   * To use ERRP_GUARD(), add it right at the beginning of the function.
>   * @errp can then be used without worrying about the argument being
>   * NULL or &error_fatal.
>
>   #define ERRP_GUARD()                                           \
>      g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};   \
>      do {                                                        \
>          if (!errp || errp == &error_fatal) {                    \
>              errp = &_auto_errp_prop.local_err;                  \
>          }                                                       \
>      } while (0)

We can make ERRP_GUARD() expand into just a declaration:

    #define ERRP_GUARD()                                            \
        g_auto(ErrorPropagator) _auto_errp_prop = {                 \
            .errp = errp,                                           \
            .local_err = ((!errp || errp == &error_fatal            \
                          ? errp = &_auto_errp_prop.local_err       \
                          : NULL),                                  \
                          NULL) }

> Or commit 5626f8c6d4 ("rcu: Add automatically released rcu_read_lock
> variants") with WITH_RCU_READ*:
>
> util/aio-posix.c:540:5: error: mixing declarations and code is 
> incompatible with standards before C99 
> [-Werror,-Wdeclaration-after-statement]
>      RCU_READ_LOCK_GUARD();
>      ^
> include/qemu/rcu.h:189:28: note: expanded from macro 'RCU_READ_LOCK_GUARD'
>      g_autoptr(RCUReadAuto) _rcu_read_auto __attribute__((unused)) = 
> rcu_read_auto_lock()
>                             ^

Valid example; RCU_READ_LOCK_GUARD() expands into a declaration.

To enable -Wdeclaration-after-statement, we'd have to futz around with
_Pragma.