Philippe Mathieu-Daudé <phi...@linaro.org> writes:
> On 6/2/24 06:53, Markus Armbruster wrote:
>> Daniel P. Berrangé <berra...@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.
