1. Patchew
  2. QEMU
  3. [PATCH 00/39] Use g_assert_not_reached instead of (g_)assert(0, false)
  4. View patch

[PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()

Pierrick Bouvier posted 39 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()
Posted by Pierrick Bouvier 2 months, 2 weeks ago 
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/ppc/spapr_events.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index cb0eeee5874..38ac1cb7866 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -645,7 +645,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
         /* we shouldn't be signaling hotplug events for resources
          * that don't support them
          */
-        g_assert(false);
+        g_assert_not_reached();
         return;
     }
 
-- 
2.39.2
Re: [PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()
Posted by BALATON Zoltan 2 months, 1 week ago 

On Tue, 10 Sep 2024, Pierrick Bouvier wrote:

> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> hw/ppc/spapr_events.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index cb0eeee5874..38ac1cb7866 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -645,7 +645,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>         /* we shouldn't be signaling hotplug events for resources
>          * that don't support them
>          */
> -        g_assert(false);
> +        g_assert_not_reached();
>         return;
>     }

If break does not make sense after g_assert_not_reached() and removed then 
return is the same here.

It may make the series shorter and easier to check that none of these are 
missed if this is done in the same patch where the assert is changed 
instead of separate patches. It's unlikely that the assert change and 
removal of the following break or return would need to be reverted 
separately so it's a simple enough change to put in one patch in my 
opinion but I don't mink if it's kept separate either.

Regards,
BALATON Zoltan
Re: [PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()
Posted by Pierrick Bouvier 2 months, 1 week ago 
On 9/11/24 07:10, BALATON Zoltan wrote:
> 
> 
> On Tue, 10 Sep 2024, Pierrick Bouvier wrote:
> 
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>> hw/ppc/spapr_events.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
>> index cb0eeee5874..38ac1cb7866 100644
>> --- a/hw/ppc/spapr_events.c
>> +++ b/hw/ppc/spapr_events.c
>> @@ -645,7 +645,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>>          /* we shouldn't be signaling hotplug events for resources
>>           * that don't support them
>>           */
>> -        g_assert(false);
>> +        g_assert_not_reached();
>>          return;
>>      }
> 
> If break does not make sense after g_assert_not_reached() and removed then
> return is the same here.
> 
> It may make the series shorter and easier to check that none of these are
> missed if this is done in the same patch where the assert is changed
> instead of separate patches. It's unlikely that the assert change and
> removal of the following break or return would need to be reverted
> separately so it's a simple enough change to put in one patch in my
> opinion but I don't mink if it's kept separate either.
> 
> Regards,
> BALATON Zoltan

Mostly done this way because it's easy for creating many commits.
Re: [PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()
Posted by BALATON Zoltan 2 months, 1 week ago 
On Wed, 11 Sep 2024, Pierrick Bouvier wrote:
> On 9/11/24 07:10, BALATON Zoltan wrote:
>> 
>> 
>> On Tue, 10 Sep 2024, Pierrick Bouvier wrote:
>> 
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>> hw/ppc/spapr_events.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
>>> index cb0eeee5874..38ac1cb7866 100644
>>> --- a/hw/ppc/spapr_events.c
>>> +++ b/hw/ppc/spapr_events.c
>>> @@ -645,7 +645,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, 
>>> uint8_t hp_action,
>>>          /* we shouldn't be signaling hotplug events for resources
>>>           * that don't support them
>>>           */
>>> -        g_assert(false);
>>> +        g_assert_not_reached();
>>>          return;
>>>      }
>> 
>> If break does not make sense after g_assert_not_reached() and removed then
>> return is the same here.
>> 
>> It may make the series shorter and easier to check that none of these are
>> missed if this is done in the same patch where the assert is changed
>> instead of separate patches. It's unlikely that the assert change and
>> removal of the following break or return would need to be reverted
>> separately so it's a simple enough change to put in one patch in my
>> opinion but I don't mink if it's kept separate either.
>> 
>> Regards,
>> BALATON Zoltan
>
> Mostly done this way because it's easy for creating many commits.

As I said I don't mind either way. Now that part of this series is queued 
it's easier to add another patch to remove the return.

Regards,
BALATON Zoltan
Re: [PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()
Posted by Daniel Henrique Barboza 2 months, 1 week ago 

On 9/10/24 7:15 PM, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   hw/ppc/spapr_events.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index cb0eeee5874..38ac1cb7866 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -645,7 +645,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>           /* we shouldn't be signaling hotplug events for resources
>            * that don't support them
>            */
> -        g_assert(false);
> +        g_assert_not_reached();
>           return;
>       }
>
Re: [PATCH 20/39] hw/ppc: replace assert(false) with g_assert_not_reached()
Posted by Richard Henderson 2 months, 2 weeks ago 
On 9/10/24 15:15, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   hw/ppc/spapr_events.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index cb0eeee5874..38ac1cb7866 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -645,7 +645,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>           /* we shouldn't be signaling hotplug events for resources
>            * that don't support them
>            */
> -        g_assert(false);
> +        g_assert_not_reached();
>           return;
>       }
>   

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

r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.