Re: [PATCH] misra: add ASSERT_UNREACHABLE() in default clauses

[Julien Grall <julien@xxxxxxx>]

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.
Anyway, my main problem with this change is that ASSERT_UNREACHABLE() is 
moved. I could possibly settle with:
default:
  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.
Cheers,

--
Julien Grall