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

[Jan Beulich <jbeulich@xxxxxxxx>]

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