| Message ID | 20240208101657.15962-1-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iothread: Simplify expression in qemu_in_iothread() | expand |
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>
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; } ---
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
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
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.
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(); }
'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(-)