Re: [PATCH] iothread: Simplify expression in qemu_in_iothread()
On 8/2/24 15:28, Peter Maydell wrote:
On Thu, 8 Feb 2024 at 14:22, Kevin Wolf 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()
On Thu, 8 Feb 2024 at 14:22, Kevin Wolf 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()
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
> > ---
> > 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é
>
> 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()
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
---
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é
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()
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
---
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
[PATCH] iothread: Simplify expression in qemu_in_iothread()
'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
---
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
