On Thu, 21 Dec 2023, Andrew Cooper wrote: > On 21/12/2023 10:29 am, Federico Serafini wrote: > > On 20/12/23 22:23, Andrew Cooper wrote: > >> On 20/12/2023 6:24 pm, Stefano Stabellini wrote: > >>> On Wed, 20 Dec 2023, Federico Serafini wrote: > >>>> On 20/12/23 12:55, Jan Beulich wrote: > >>>>> On 20.12.2023 12:48, Julien Grall wrote: > >>>>>> On 20/12/2023 11:03, Federico Serafini wrote: > >>>>>>> --- a/xen/arch/arm/arm64/vsysreg.c > >>>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c > >>>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > >>>>>>> /* RO at EL0. RAZ/WI at EL1 */ > >>>>>>> if ( regs_mode_is_user(regs) ) > >>>>>>> return handle_ro_raz(regs, regidx, > >>>>>>> hsr.sysreg.read, hsr, > >>>>>>> 0); > >>>>>>> - else > >>>>>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, > >>>>>>> hsr, > >>>>>>> 1); > >>>>>>> + > >>>>>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, > >>>>>>> hsr, 1); > >>>>>> I don't 100% like this change (mostly because I find if/else clearer > >>>>>> here). > >>>>> While (it doesn't matter here) my view on this is different, I'm > >>>>> still > >>>>> puzzled why the tool would complain / why a change here is necessary. > >>>>> It is not _one_ return statement, but there's still (and > >>>>> obviously) no > >>>>> way of falling through. > >>>> The tool is configurable: > >>>> if you prefer deviate these cases instead of refactoring the code > >>>> I can update the configuration. > >>> If you say "deviation", it implies that there is a violation. I think > >>> Jan's point was that both code versions shouldn't be any different. > >>> > >>> # option-1 > >>> if (a) > >>> return f1(); > >>> else > >>> return f2(); > >>> > >>> # option-2 > >>> if (a) > >>> return f1(); > >>> return f2(); > >>> > >>> Both options are equally guaranteed to always return. It looks like a > >>> peculiar limitation to only recognize option-2 as something that > >>> returns > >>> 100% of the times. Also option-1 returns 100% of the times. What am I > >>> missing? > > > > I don't think this is necessarily a limitation because it highlights > > cases where an if-else could be replaced with a simple if: > > some may find an if-else clearer, other may find the single if clearer. > > > > From a safety point of view both options are safe because there > > is no risk of unintentional fall through. > > > > If you all agree on the fact that small code refactoring like the ones I > > proposed are counterproductive, then I can update the tool configuration > > to consider also option-1 as safe. > > I would certainly prefer if we could continue to use option 1.
Yes, that is my preference too
