On 13.08.2025 00:54, Julien Grall wrote: > Hi Jan, > > On 12/08/2025 08:32, Jan Beulich wrote: >> On 11.08.2025 23:21, Julien Grall wrote: >>> On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote: >>>> --- a/xen/common/grant_table.c >>>> +++ b/xen/common/grant_table.c >>>> @@ -330,9 +330,12 @@ shared_entry_header(struct grant_table *t, >>>> grant_ref_t ref) >>>> /* Returned values should be independent of speculative >>>> execution */ >>>> block_speculation(); >>>> return &shared_entry_v2(t, ref).hdr; >>>> + >>>> + default: >>>> + ASSERT_UNREACHABLE(); >>>> + break; >>>> } >>>> >>>> - ASSERT_UNREACHABLE(); >>> > block_speculation();> >>>> return NULL; >>> >>> I know you are trying to apply the MISRA rule. But this is odd that you >>> move the ASSERT_UNREACHABLE() but then code after is still only >>> reachable from the default. In fact, this is introducing a risk if >>> someone decides to add a new case but then forgot to return a value. >>> >>> By moving the two other lines, the compiler should be able to throw an >>> error if you forgot a return. >> >> I think we did discuss this pattern in the past. While moving everything up >> to the "return" into the default: handling will please Eclair / Misra, we'll >> then end up with no return statement at the end of a non-void function. >> Beyond being good practice (imo) to have such a "main" return statement, >> that's actually another rule, just one we apparently didn't accept (15.5). > > Reading 15.5, this seems to be about having a single return in the > function. Unless I misunderstood something, this is different from what > you suggest.
Sue, the connection is a lose one. What I mean is that adding yet another return _not_ at the end of the function moves use further away from 15.5 compliance. > Anyway, my main problem with this change is that ASSERT_UNREACHABLE() is > moved. I could possibly settle with: > > default: > break; > } Which would violate another rule, iirc (demanding that there be more than just "break"). > ASSERT_UNREACHABLE(); > ... > > But at least to me, this pattern is more difficult to read because I > have to look through the switch to understand the patch is only meant ot > be used by the "default" case. +1 Jan
