Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall <Julien.Grall@xxxxxxx>]

On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>> BTW, I checked the series with -Wswitch-default:
>>>>> -Wswitch-default
>>>>> Warn whenever a switch statement does not have a default case.
>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>> to follow
>>>>> the existing "error handling" at those places.
>>>>
>>>> It is not because BUG() is been used today in some places that we need to
>>>> continue to spread it.
>>>>
>>>>> Use of BUG() itself is another topic which will also need to be
>>>>> addressed
>>>>
>>>> So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>>
>> That's not how it works in upstream. If you know some constructs are wrong, 
>> it
>> is best to try to address partially the problem directly then having so you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> 
> Hi Oleksandr, Julien,
> 
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
> 
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

I looked at the first patch and to be honest I can't see how this hence 
our code...

> 
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
> 
>    default:
>    /* unreachable because blah and blah */

... as we already have defensive code. Indeed, in most of the switch we 
deal with potential issue by initializing the variable before the 
switch. If you look at the first patch, a lot of "default: break;" is 
introduced. So what's our benefits? How this code is more defensive than 
what we currently have?

Furthermore, how this is going to help us (thanks to -Wswitch) if an 
enumerate is extended and we miss a case that the compiler don't notice 
anymore?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel