* [PATCH] docs/style: allow C99 mixed declarations @ 2024-02-05 17:18 Stefan Hajnoczi 2024-02-05 17:41 ` Daniel P. Berrangé ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2024-02-05 17:18 UTC (permalink / raw) To: qemu-devel Cc: Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Daniel P. Berrangé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée, Stefan Hajnoczi 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. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- docs/devel/style.rst | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 2f68b50079..80c4e4df52 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -199,26 +199,6 @@ Rationale: a consistent (except for functions...) bracing style reduces ambiguity and avoids needless churn when lines are added or removed. Furthermore, it is the QEMU coding style. -Declarations -============ - -Mixed declarations (interleaving statements and declarations within -blocks) are generally not allowed; declarations should be at the beginning -of blocks. To avoid accidental re-use it is permissible to declare -loop variables inside for loops: - -.. code-block:: c - - for (int i = 0; i < ARRAY_SIZE(thing); i++) { - /* do something loopy */ - } - -Every now and then, an exception is made for declarations inside a -#ifdef or #ifndef block: if the code looks nicer, such declarations can -be placed at the top of the block even if there are statements above. -On the other hand, however, it's often best to move that #ifdef/#ifndef -block to a separate function altogether. - Conditional statements ====================== -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 17:18 [PATCH] docs/style: allow C99 mixed declarations Stefan Hajnoczi @ 2024-02-05 17:41 ` Daniel P. Berrangé 2024-02-05 17:55 ` Peter Maydell ` (2 more replies) 2024-02-05 23:17 ` Alex Bennée 2024-02-06 13:48 ` Stefan Hajnoczi 2 siblings, 3 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2024-02-05 17:41 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée 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. IIRC, we had a discussion on this topic sometime last year, but can't remember what the $SUBJECT was, so I'll just repeat what I said then. Combining C99 mixed declarations with 'goto' is dangerous. Consider this program: $ cat jump.c #include <stdlib.h> int main(int argc, char**argv) { if (getenv("SKIP")) goto target; char *foo = malloc(30); target: free(foo); } $ gcc -Wall -Wuninitialized -o jump jump.c $ SKIP=1 ./jump free(): invalid pointer Aborted (core dumped) -> The programmer thinks they have initialized 'foo' -> GCC thinks the programmer has initialized 'foo' -> Yet 'foo' is not guaranteed to be initialized at 'target:' Given that QEMU makes heavy use of 'goto', allowing C99 mixed declarations exposes us to significant danger. Full disclosure, GCC fails to diagnmose this mistake, even with a decl at start of 'main', but at least the mistake is now more visible to the programmer. Fortunately with -fanalyzer GCC can diagnose this: $ gcc -fanalyzer -Wall -o jump jump.c jump.c: In function ‘main’: jump.c:12:3: warning: use of uninitialized value ‘foo’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value] 12 | free(foo); | ^~~~~~~~~ ‘main’: events 1-5 | | 6 | if (getenv("SKIP")) | | ~ | | | | | (3) following ‘true’ branch... | 7 | goto target; | | ~~~~ | | | | | (4) ...to here | 8 | | 9 | char *foo = malloc(30); | | ^~~ | | | | | (1) region created on stack here | | (2) capacity: 8 bytes |...... | 12 | free(foo); | | ~~~~~~~~~ | | | | | (5) use of uninitialized value ‘foo’ here ...but -fanalyzer isn't something we have enabled by default, it is opt-in. I'm also not sure how comprehensive the flow control analysis of -fanalyzer is ? Can we be sure it'll catch these mistakes in large complex functions with many code paths ? 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. 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 while C99 mixed decl is appealing, it isn't exactly a game changer in improving code maintainability. > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > docs/devel/style.rst | 20 -------------------- > 1 file changed, 20 deletions(-) > > diff --git a/docs/devel/style.rst b/docs/devel/style.rst > index 2f68b50079..80c4e4df52 100644 > --- a/docs/devel/style.rst > +++ b/docs/devel/style.rst > @@ -199,26 +199,6 @@ Rationale: a consistent (except for functions...) bracing style reduces > ambiguity and avoids needless churn when lines are added or removed. > Furthermore, it is the QEMU coding style. > > -Declarations > -============ > - > -Mixed declarations (interleaving statements and declarations within > -blocks) are generally not allowed; declarations should be at the beginning > -of blocks. To avoid accidental re-use it is permissible to declare > -loop variables inside for loops: > - > -.. code-block:: c > - > - for (int i = 0; i < ARRAY_SIZE(thing); i++) { > - /* do something loopy */ > - } > - > -Every now and then, an exception is made for declarations inside a > -#ifdef or #ifndef block: if the code looks nicer, such declarations can > -be placed at the top of the block even if there are statements above. > -On the other hand, however, it's often best to move that #ifdef/#ifndef > -block to a separate function altogether. > - > Conditional statements > ====================== > > -- > 2.43.0 > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 17:41 ` Daniel P. Berrangé @ 2024-02-05 17:55 ` Peter Maydell 2024-02-05 18:12 ` Samuel Tardieu 2024-02-06 5:53 ` Markus Armbruster 2 siblings, 0 replies; 11+ messages in thread From: Peter Maydell @ 2024-02-05 17:55 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Stefan Hajnoczi, qemu-devel, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Richard Henderson, Thomas Huth, Alex Bennée On Mon, 5 Feb 2024 at 17:41, Daniel P. Berrangé <berrange@redhat.com> wrote: > 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 > while C99 mixed decl is appealing, it isn't exactly a game > changer in improving code maintainability. I definitely think we should either (a) say we're OK with mixed declarations or (b) enforce it via the warning option. (gcc -Wdeclaration-after-statement does not appear to warn for the "declaration inside for()" syntax, so we could enable it if we wanted to fix the existing style lapses.) Personally I prefer declaration-at-top-of-block, but more as an aesthetic choice than anything else. thanks -- PMM ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 17:41 ` Daniel P. Berrangé 2024-02-05 17:55 ` Peter Maydell @ 2024-02-05 18:12 ` Samuel Tardieu 2024-02-05 19:06 ` Stefan Hajnoczi 2024-02-06 5:53 ` Markus Armbruster 2 siblings, 1 reply; 11+ messages in thread From: Samuel Tardieu @ 2024-02-05 18:12 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Stefan Hajnoczi, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée, qemu-devel Daniel P. Berrangé <berrange@redhat.com> writes: > $ gcc -Wall -Wuninitialized -o jump jump.c Note that many GCC warnings don't trigger if you don't enable optimizations. In the case you exhibit, adding -O is enough to get a sensible warning: $ gcc -Wall -O -o jump jump.c jump.c: In function ‘main’: jump.c:11:3: warning: ‘foo’ may be used uninitialized [-Wmaybe-uninitialized] 11 | free(foo); | ^~~~~~~~~ jump.c:8:9: note: ‘foo’ was declared here 8 | char *foo = malloc(30); | ^~~ Best. Sam -- Samuel Tardieu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 18:12 ` Samuel Tardieu @ 2024-02-05 19:06 ` Stefan Hajnoczi 2024-02-05 19:17 ` Daniel P. Berrangé 0 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2024-02-05 19:06 UTC (permalink / raw) To: Samuel Tardieu Cc: Daniel P. Berrangé, Stefan Hajnoczi, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée, qemu-devel On Mon, 5 Feb 2024 at 13:16, Samuel Tardieu <sam@rfc1149.net> wrote: > > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > $ gcc -Wall -Wuninitialized -o jump jump.c > > Note that many GCC warnings don't trigger if you don't enable > optimizations. In the case you exhibit, adding -O is enough to get > a sensible warning: > > $ gcc -Wall -O -o jump jump.c > jump.c: In function ‘main’: > jump.c:11:3: warning: ‘foo’ may be used uninitialized > [-Wmaybe-uninitialized] > 11 | free(foo); > | ^~~~~~~~~ > jump.c:8:9: note: ‘foo’ was declared here > 8 | char *foo = malloc(30); > | ^~~ llvm also prints a warning: jump.c:5:7: warning: variable 'foo' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] I confirmed that QEMU's current compiler flags enable these warnings so both gcc and llvm detect the issue that Daniel pointed out in QEMU code. Daniel: Does this address your concern about compiler warnings? Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 19:06 ` Stefan Hajnoczi @ 2024-02-05 19:17 ` Daniel P. Berrangé 0 siblings, 0 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2024-02-05 19:17 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Samuel Tardieu, Stefan Hajnoczi, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée, qemu-devel On Mon, Feb 05, 2024 at 02:06:39PM -0500, Stefan Hajnoczi wrote: > On Mon, 5 Feb 2024 at 13:16, Samuel Tardieu <sam@rfc1149.net> wrote: > > > > > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > > > $ gcc -Wall -Wuninitialized -o jump jump.c > > > > Note that many GCC warnings don't trigger if you don't enable > > optimizations. In the case you exhibit, adding -O is enough to get > > a sensible warning: > > > > $ gcc -Wall -O -o jump jump.c > > jump.c: In function ‘main’: > > jump.c:11:3: warning: ‘foo’ may be used uninitialized > > [-Wmaybe-uninitialized] > > 11 | free(foo); > > | ^~~~~~~~~ > > jump.c:8:9: note: ‘foo’ was declared here > > 8 | char *foo = malloc(30); > > | ^~~ > > llvm also prints a warning: > > jump.c:5:7: warning: variable 'foo' is used uninitialized whenever > 'if' condition is true [-Wsometimes-uninitialized] > > I confirmed that QEMU's current compiler flags enable these warnings > so both gcc and llvm detect the issue that Daniel pointed out in QEMU > code. > > Daniel: Does this address your concern about compiler warnings? While it is good that its included when optimization is enabled, IME GCC is not entirely reliable at detecting unintialized variables as it has limits on extent of its flow control analysis. Also consider the maint impact if a method includes C99 decls, and a contributor later needs to add a 'goto' error cleanup path. They might be forced to make a bunch of unrelated refactoring changes to eliminate use of the C99 decls. This makes me not a fan of having a situation where we tell contributors they can use C99 mixed decls, except when they can't use them. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 17:41 ` Daniel P. Berrangé 2024-02-05 17:55 ` Peter Maydell 2024-02-05 18:12 ` Samuel Tardieu @ 2024-02-06 5:53 ` Markus Armbruster 2024-02-07 5:28 ` Philippe Mathieu-Daudé 2 siblings, 1 reply; 11+ messages in thread From: Markus Armbruster @ 2024-02-06 5:53 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Stefan Hajnoczi, qemu-devel, Hanna Czenczek, Philippe Mathieu-Daudé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée 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. > > IIRC, we had a discussion on this topic sometime last year, but can't > remember what the $SUBJECT was, so I'll just repeat what I said then. From: Juan Quintela <quintela@redhat.com> Subject: [PATCH] Change the default for Mixed declarations. Date: Tue, 14 Feb 2023 17:07:38 +0100 Message-Id: <20230214160738.88614-1-quintela@redhat.com> https://lore.kernel.org/qemu-devel/20230214160738.88614-1-quintela@redhat.com/ > Combining C99 mixed declarations with 'goto' is dangerous. > > Consider this program: > > $ cat jump.c > #include <stdlib.h> > > int main(int argc, char**argv) { > > if (getenv("SKIP")) > goto target; > > char *foo = malloc(30); > > target: > free(foo); > } > > $ gcc -Wall -Wuninitialized -o jump jump.c > > $ SKIP=1 ./jump > free(): invalid pointer > Aborted (core dumped) > > > -> The programmer thinks they have initialized 'foo' > -> GCC thinks the programmer has initialized 'foo' > -> Yet 'foo' is not guaranteed to be initialized at 'target:' > > Given that QEMU makes heavy use of 'goto', allowing C99 mixed > declarations exposes us to significant danger. > > Full disclosure, GCC fails to diagnmose this mistake, even > with a decl at start of 'main', but at least the mistake is > now more visible to the programmer. > > Fortunately with -fanalyzer GCC can diagnose this: > > $ gcc -fanalyzer -Wall -o jump jump.c > jump.c: In function ‘main’: > jump.c:12:3: warning: use of uninitialized value ‘foo’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value] > 12 | free(foo); > | ^~~~~~~~~ > ‘main’: events 1-5 > | > | 6 | if (getenv("SKIP")) > | | ~ > | | | > | | (3) following ‘true’ branch... > | 7 | goto target; > | | ~~~~ > | | | > | | (4) ...to here > | 8 | > | 9 | char *foo = malloc(30); > | | ^~~ > | | | > | | (1) region created on stack here > | | (2) capacity: 8 bytes > |...... > | 12 | free(foo); > | | ~~~~~~~~~ > | | | > | | (5) use of uninitialized value ‘foo’ here > > > ...but -fanalyzer isn't something we have enabled by default, it > is opt-in. I'm also not sure how comprehensive the flow control > analysis of -fanalyzer is ? Can we be sure it'll catch these > mistakes in large complex functions with many code paths ? > > 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. > it isn't exactly a game > changer in improving code maintainability. [...] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-06 5:53 ` Markus Armbruster @ 2024-02-07 5:28 ` Philippe Mathieu-Daudé 2024-02-07 7:37 ` Markus Armbruster 0 siblings, 1 reply; 11+ messages in thread From: Philippe Mathieu-Daudé @ 2024-02-07 5:28 UTC (permalink / raw) To: Markus Armbruster, Daniel P. Berrangé Cc: Stefan Hajnoczi, qemu-devel, Hanna Czenczek, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée, Vladimir Sementsov-Ogievskiy 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) 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() ^ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-07 5:28 ` Philippe Mathieu-Daudé @ 2024-02-07 7:37 ` Markus Armbruster 0 siblings, 0 replies; 11+ messages in thread From: Markus Armbruster @ 2024-02-07 7:37 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Daniel P. Berrangé, Stefan Hajnoczi, qemu-devel, Hanna Czenczek, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée, Vladimir Sementsov-Ogievskiy 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 17:18 [PATCH] docs/style: allow C99 mixed declarations Stefan Hajnoczi 2024-02-05 17:41 ` Daniel P. Berrangé @ 2024-02-05 23:17 ` Alex Bennée 2024-02-06 13:48 ` Stefan Hajnoczi 2 siblings, 0 replies; 11+ messages in thread From: Alex Bennée @ 2024-02-05 23:17 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Daniel P. Berrangé, Peter Maydell, Richard Henderson, Thomas Huth Stefan Hajnoczi <stefanha@redhat.com> writes: > 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. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > docs/devel/style.rst | 20 -------------------- > 1 file changed, 20 deletions(-) While I may be convinced there may be some cases where mixed declarations could make things simpler/clearer I don't support removing all the guidance and leaving it as a free for all as long as the compiler is happy. -- Alex Bennée Virtualisation Tech Lead @ Linaro ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] docs/style: allow C99 mixed declarations 2024-02-05 17:18 [PATCH] docs/style: allow C99 mixed declarations Stefan Hajnoczi 2024-02-05 17:41 ` Daniel P. Berrangé 2024-02-05 23:17 ` Alex Bennée @ 2024-02-06 13:48 ` Stefan Hajnoczi 2 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2024-02-06 13:48 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-devel, Hanna Czenczek, Markus Armbruster, Philippe Mathieu-Daudé, Daniel P. Berrangé, Peter Maydell, Richard Henderson, Thomas Huth, Alex Bennée On Mon, 5 Feb 2024 at 12:19, Stefan Hajnoczi <stefanha@redhat.com> 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. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > docs/devel/style.rst | 20 -------------------- > 1 file changed, 20 deletions(-) I'm withdrawing this patch because it seems many of the other QEMU maintainers prefer not to have C99 mixed declarations. Stefan > diff --git a/docs/devel/style.rst b/docs/devel/style.rst > index 2f68b50079..80c4e4df52 100644 > --- a/docs/devel/style.rst > +++ b/docs/devel/style.rst > @@ -199,26 +199,6 @@ Rationale: a consistent (except for functions...) bracing style reduces > ambiguity and avoids needless churn when lines are added or removed. > Furthermore, it is the QEMU coding style. > > -Declarations > -============ > - > -Mixed declarations (interleaving statements and declarations within > -blocks) are generally not allowed; declarations should be at the beginning > -of blocks. To avoid accidental re-use it is permissible to declare > -loop variables inside for loops: > - > -.. code-block:: c > - > - for (int i = 0; i < ARRAY_SIZE(thing); i++) { > - /* do something loopy */ > - } > - > -Every now and then, an exception is made for declarations inside a > -#ifdef or #ifndef block: if the code looks nicer, such declarations can > -be placed at the top of the block even if there are statements above. > -On the other hand, however, it's often best to move that #ifdef/#ifndef > -block to a separate function altogether. > - > Conditional statements > ====================== > > -- > 2.43.0 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-02-07 7:38 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-05 17:18 [PATCH] docs/style: allow C99 mixed declarations Stefan Hajnoczi 2024-02-05 17:41 ` Daniel P. Berrangé 2024-02-05 17:55 ` Peter Maydell 2024-02-05 18:12 ` Samuel Tardieu 2024-02-05 19:06 ` Stefan Hajnoczi 2024-02-05 19:17 ` Daniel P. Berrangé 2024-02-06 5:53 ` Markus Armbruster 2024-02-07 5:28 ` Philippe Mathieu-Daudé 2024-02-07 7:37 ` Markus Armbruster 2024-02-05 23:17 ` Alex Bennée 2024-02-06 13:48 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).