| Message ID | 20240910221606.1817478-21-pierrick.bouvier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use g_assert_not_reached instead of (g_)assert(0, false) | expand |
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~
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; > } >
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
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.
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
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; }
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- hw/ppc/spapr_events.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)