1. Patchew
  2. QEMU
  3. View series

[PATCH] iothread: Simplify expression in qemu_in_iothread()

Kevin Wolf posted 1 patch 9 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240208101657.15962-1-kwolf@redhat.com
iothread.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[PATCH] iothread: Simplify expression in qemu_in_iothread()
Posted by Kevin Wolf 9 months, 3 weeks ago 
'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
Use the more obvious way to write it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 iothread.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/iothread.c b/iothread.c
index 6c1fc8c856..e1e9e04736 100644
--- a/iothread.c
+++ b/iothread.c
@@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
 
 bool qemu_in_iothread(void)
 {
-    return qemu_get_current_aio_context() == qemu_get_aio_context() ?
-                    false : true;
+    return qemu_get_current_aio_context() != qemu_get_aio_context();
 }
-- 
2.43.0
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Posted by Philippe Mathieu-Daudé 9 months, 3 weeks ago 
On 8/2/24 11:16, Kevin Wolf wrote:
> 'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
> Use the more obvious way to write it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   iothread.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/iothread.c b/iothread.c
> index 6c1fc8c856..e1e9e04736 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
>   
>   bool qemu_in_iothread(void)
>   {
> -    return qemu_get_current_aio_context() == qemu_get_aio_context() ?
> -                    false : true;
> +    return qemu_get_current_aio_context() != qemu_get_aio_context();
>   }

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

BTW using the same pattern:

-- >8 --
diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
index ec98456e5d..d074762a25 100644
--- a/hw/nvram/xlnx-zynqmp-efuse.c
+++ b/hw/nvram/xlnx-zynqmp-efuse.c
@@ -582,7 +582,7 @@ static uint64_t 
zynqmp_efuse_cache_load_prew(RegisterInfo *reg,

  static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
  {
-    return val == 0xDF0D ? 0 : 1;
+    return val != 0xDF0D;
  }

diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
index 301e61d0dd..bdd73bd181 100644
--- a/tests/tcg/aarch64/sysregs.c
+++ b/tests/tcg/aarch64/sysregs.c
@@ -183,5 +183,5 @@ int main(void)
          return 1;
      }

-    return should_fail_count == 6 ? 0 : 1;
+    return should_fail_count != 6;
  }
---
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Posted by Kevin Wolf 9 months, 3 weeks ago 
Am 08.02.2024 um 11:48 hat Philippe Mathieu-Daudé geschrieben:
> On 8/2/24 11:16, Kevin Wolf wrote:
> > 'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
> > Use the more obvious way to write it.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   iothread.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/iothread.c b/iothread.c
> > index 6c1fc8c856..e1e9e04736 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
> >   bool qemu_in_iothread(void)
> >   {
> > -    return qemu_get_current_aio_context() == qemu_get_aio_context() ?
> > -                    false : true;
> > +    return qemu_get_current_aio_context() != qemu_get_aio_context();
> >   }
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> BTW using the same pattern:
> 
> -- >8 --
> diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
> index ec98456e5d..d074762a25 100644
> --- a/hw/nvram/xlnx-zynqmp-efuse.c
> +++ b/hw/nvram/xlnx-zynqmp-efuse.c
> @@ -582,7 +582,7 @@ static uint64_t
> zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
> 
>  static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
>  {
> -    return val == 0xDF0D ? 0 : 1;
> +    return val != 0xDF0D;
>  }

Maybe. I would have to know that device to tell if this is really meant
as boolean. Or maybe it should be written 0x0 and 0x1 to signify that
it's a register value or something.

> diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
> index 301e61d0dd..bdd73bd181 100644
> --- a/tests/tcg/aarch64/sysregs.c
> +++ b/tests/tcg/aarch64/sysregs.c
> @@ -183,5 +183,5 @@ int main(void)
>          return 1;
>      }
> 
> -    return should_fail_count == 6 ? 0 : 1;
> +    return should_fail_count != 6;
>  }

This one isn't unclear to me, though. This is EXIT_SUCCESS and
EXIT_FAILURE, just open-coded. I think making your change would make it
only more confusing.

Kevin
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Posted by Peter Maydell 9 months, 3 weeks ago 
On Thu, 8 Feb 2024 at 14:22, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 08.02.2024 um 11:48 hat Philippe Mathieu-Daudé geschrieben:
> > BTW using the same pattern:
> >
> > -- >8 --
> > diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
> > index ec98456e5d..d074762a25 100644
> > --- a/hw/nvram/xlnx-zynqmp-efuse.c
> > +++ b/hw/nvram/xlnx-zynqmp-efuse.c
> > @@ -582,7 +582,7 @@ static uint64_t
> > zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
> >
> >  static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
> >  {
> > -    return val == 0xDF0D ? 0 : 1;
> > +    return val != 0xDF0D;
> >  }
>
> Maybe. I would have to know that device to tell if this is really meant
> as boolean. Or maybe it should be written 0x0 and 0x1 to signify that
> it's a register value or something.

This is a RegisterAccessinfo pre_write hook. The docs say:
 * @pre_write: Pre write callback. Passed the value that's to be written,
 * immediately before the actual write. The returned value is what is written,
 * giving the handler a chance to modify the written value.

So it is indeed returning a register value, not a boolean flag
masquerading as a uint64_t.

> > diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
> > index 301e61d0dd..bdd73bd181 100644
> > --- a/tests/tcg/aarch64/sysregs.c
> > +++ b/tests/tcg/aarch64/sysregs.c
> > @@ -183,5 +183,5 @@ int main(void)
> >          return 1;
> >      }
> >
> > -    return should_fail_count == 6 ? 0 : 1;
> > +    return should_fail_count != 6;
> >  }
>
> This one isn't unclear to me, though. This is EXIT_SUCCESS and
> EXIT_FAILURE, just open-coded. I think making your change would make it
> only more confusing.

I agree on this one.

-- PMM
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Posted by Philippe Mathieu-Daudé 9 months, 3 weeks ago 
On 8/2/24 15:28, Peter Maydell wrote:
> On Thu, 8 Feb 2024 at 14:22, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> Am 08.02.2024 um 11:48 hat Philippe Mathieu-Daudé geschrieben:
>>> BTW using the same pattern:
>>>
>>> -- >8 --
>>> diff --git a/hw/nvram/xlnx-zynqmp-efuse.c b/hw/nvram/xlnx-zynqmp-efuse.c
>>> index ec98456e5d..d074762a25 100644
>>> --- a/hw/nvram/xlnx-zynqmp-efuse.c
>>> +++ b/hw/nvram/xlnx-zynqmp-efuse.c
>>> @@ -582,7 +582,7 @@ static uint64_t
>>> zynqmp_efuse_cache_load_prew(RegisterInfo *reg,
>>>
>>>   static uint64_t zynqmp_efuse_wr_lock_prew(RegisterInfo *reg, uint64_t val)
>>>   {
>>> -    return val == 0xDF0D ? 0 : 1;
>>> +    return val != 0xDF0D;
>>>   }
>>
>> Maybe. I would have to know that device to tell if this is really meant
>> as boolean. Or maybe it should be written 0x0 and 0x1 to signify that
>> it's a register value or something.
> 
> This is a RegisterAccessinfo pre_write hook. The docs say:
>   * @pre_write: Pre write callback. Passed the value that's to be written,
>   * immediately before the actual write. The returned value is what is written,
>   * giving the handler a chance to modify the written value.
> 
> So it is indeed returning a register value, not a boolean flag
> masquerading as a uint64_t.
> 
>>> diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
>>> index 301e61d0dd..bdd73bd181 100644
>>> --- a/tests/tcg/aarch64/sysregs.c
>>> +++ b/tests/tcg/aarch64/sysregs.c
>>> @@ -183,5 +183,5 @@ int main(void)
>>>           return 1;
>>>       }
>>>
>>> -    return should_fail_count == 6 ? 0 : 1;
>>> +    return should_fail_count != 6;
>>>   }
>>
>> This one isn't unclear to me, though. This is EXIT_SUCCESS and
>> EXIT_FAILURE, just open-coded. I think making your change would make it
>> only more confusing.
> 
> I agree on this one.

Thanks for both analysis and sorry for my confusion,
I was just looking at the pattern without interpreting
each proper use case.
Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
Posted by Laurent Vivier 9 months, 3 weeks ago 
Le 08/02/2024 à 11:16, Kevin Wolf a écrit :
> 'a == b ? false : true' is a rather convoluted way of writing 'a != b'.
> Use the more obvious way to write it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   iothread.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/iothread.c b/iothread.c
> index 6c1fc8c856..e1e9e04736 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -404,6 +404,5 @@ IOThread *iothread_by_id(const char *id)
>   
>   bool qemu_in_iothread(void)
>   {
> -    return qemu_get_current_aio_context() == qemu_get_aio_context() ?
> -                    false : true;
> +    return qemu_get_current_aio_context() != qemu_get_aio_context();
>   }

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.