[RFC PATCH] docs/style: permit inline loop variables

[RFC PATCH] docs/style: permit inline loop variables

[Alex Bennée]

I've already wasted enough of my time debugging aliased variables in
deeply nested loops. While not scattering variable declarations around
is a good aim I think we can make an exception for stuff used inside a
loop.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/style.rst | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 3cfcdeb9cd..2f68b50079 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -204,7 +204,14 @@ Declarations
 
 Mixed declarations (interleaving statements and declarations within
 blocks) are generally not allowed; declarations should be at the beginning
-of blocks.
+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
-- 
2.39.2

Re: [RFC PATCH] docs/style: permit inline loop variables

[Stefan Hajnoczi]

On Tue, 22 Aug 2023 at 11:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> I've already wasted enough of my time debugging aliased variables in
> deeply nested loops. While not scattering variable declarations around
> is a good aim I think we can make an exception for stuff used inside a
> loop.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/style.rst | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

>
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 3cfcdeb9cd..2f68b50079 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -204,7 +204,14 @@ Declarations
>
>  Mixed declarations (interleaving statements and declarations within
>  blocks) are generally not allowed; declarations should be at the beginning
> -of blocks.
> +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
> --
> 2.39.2
>
>

Re: [RFC PATCH] docs/style: permit inline loop variables

[Peter Maydell]

On Tue, 22 Aug 2023 at 16:50, Alex Bennée <alex.bennee@linaro.org> wrote:
> I've already wasted enough of my time debugging aliased variables in
> deeply nested loops.

In theory we could try to enable -Wshadow and deal with
all the existing cases of aliasing, which would then
allow us to turn it into an error and catch your bugs :-)

Anyway, I think declaration-in-for-loop is OK and we
already have quite a lot of instances of it.

-- PMM

Re: [RFC PATCH] docs/style: permit inline loop variables

[Richard Henderson]

On 8/22/23 08:50, Alex Bennée wrote:
> I've already wasted enough of my time debugging aliased variables in
> deeply nested loops. While not scattering variable declarations around
> is a good aim I think we can make an exception for stuff used inside a
> loop.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   docs/devel/style.rst | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH] docs/style: permit inline loop variables

[Thomas Huth]

On 22/08/2023 17.50, Alex Bennée wrote:
> I've already wasted enough of my time debugging aliased variables in
> deeply nested loops. While not scattering variable declarations around
> is a good aim I think we can make an exception for stuff used inside a
> loop.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/devel/style.rst | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 3cfcdeb9cd..2f68b50079 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -204,7 +204,14 @@ Declarations
>   
>   Mixed declarations (interleaving statements and declarations within
>   blocks) are generally not allowed; declarations should be at the beginning
> -of blocks.
> +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 */
> +    }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [RFC PATCH] docs/style: permit inline loop variables

[Philippe Mathieu-Daudé]

On 22/8/23 17:50, Alex Bennée wrote:
> I've already wasted enough of my time debugging aliased variables in
> deeply nested loops. While not scattering variable declarations around
> is a good aim I think we can make an exception for stuff used inside a
> loop.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/devel/style.rst | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 3cfcdeb9cd..2f68b50079 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -204,7 +204,14 @@ Declarations
>   
>   Mixed declarations (interleaving statements and declarations within
>   blocks) are generally not allowed; declarations should be at the beginning
> -of blocks.
> +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++) {

ARRAY_SIZE() -> sizeof() -> size_t -> unsigned.

Is it a good example to use 'int' here?

Otherwise, glad to declare variables in loops!

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

> +        /* 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

Re: [RFC PATCH] docs/style: permit inline loop variables

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 22 Aug 2023 at 16:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>> I've already wasted enough of my time debugging aliased variables in
>> deeply nested loops.
>
> In theory we could try to enable -Wshadow and deal with
> all the existing cases of aliasing, which would then
> allow us to turn it into an error and catch your bugs :-)

In practice, a quick compile with -Wshadow -Wno-error=shadow coughs up
almost 6000 warnings.  There are duplicates since we compile many files
multiple times, so I piped through sort -u | wc -l, and got about 1200.

> Anyway, I think declaration-in-for-loop is OK and we
> already have quite a lot of instances of it.

Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [RFC PATCH] docs/style: permit inline loop variables

[Peter Maydell]

On Wed, 23 Aug 2023 at 06:59, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 22 Aug 2023 at 16:50, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> I've already wasted enough of my time debugging aliased variables in
> >> deeply nested loops.
> >
> > In theory we could try to enable -Wshadow and deal with
> > all the existing cases of aliasing, which would then
> > allow us to turn it into an error and catch your bugs :-)
>
> In practice, a quick compile with -Wshadow -Wno-error=shadow coughs up
> almost 6000 warnings.  There are duplicates since we compile many files
> multiple times, so I piped through sort -u | wc -l, and got about 1200.

-Wshadow=local has only 211 non-duplicate warnings, which
is almost tractable...

(A lot of the duplicates are from local variables declared in macros
like MAX(), MIN() and QOBJECT(), when those macros are used in a nested
way, like MIN(MIN(x,y),z). We could deal with those by using the
__COUNTER__ trick, I guess.)

-- PMM

Re: [RFC PATCH] docs/style: permit inline loop variables

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 23 Aug 2023 at 06:59, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Tue, 22 Aug 2023 at 16:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >> I've already wasted enough of my time debugging aliased variables in
>> >> deeply nested loops.
>> >
>> > In theory we could try to enable -Wshadow and deal with
>> > all the existing cases of aliasing, which would then
>> > allow us to turn it into an error and catch your bugs :-)
>>
>> In practice, a quick compile with -Wshadow -Wno-error=shadow coughs up
>> almost 6000 warnings.  There are duplicates since we compile many files
>> multiple times, so I piped through sort -u | wc -l, and got about 1200.
>
> -Wshadow=local has only 211 non-duplicate warnings, which
> is almost tractable...
>
> (A lot of the duplicates are from local variables declared in macros
> like MAX(), MIN() and QOBJECT(), when those macros are used in a nested
> way, like MIN(MIN(x,y),z). We could deal with those by using the
> __COUNTER__ trick, I guess.)

Oooh, preprocessor trickery!

I posted "[PATCH 0/7] Steps towards enabling -Wshadow=local".  Need help
to get the job finished.