On Thu, 31 Oct 2024 at 10:52, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Thu, 31 Oct 2024 at 00:32, Dr. David Alan Gilbert <d...@treblig.org> wrote: > > > > * Phil Dennis-Jordan (li...@philjordan.eu) wrote: > > > On Wed 30. Oct 2024 at 15:09, Anastasia Belova <abel...@astralinux.ru> > > > wrote: > > > > > > > default case has no condition. So if it is placed > > > > higher that other cases, they are unreachable. > > > > > > > > Move dafult case down. > > > > > > > > > > The stylistic merits might be debatable, but: the order of cases in a > > > switch block in C does not matter, the default case can appear anywhere. > > > The other cases are still reachable. So at minimum, the commit message is > > > incorrect. > > > > I'd agree; the analysis is wrong - it works as intended. > > As for style, I'd normally agree that 'default' at end makes sense, > > but: > > a) I hate duplicating code > > b) in a way this reads nicely: > > default: > > case 1: > > > > 'default is the same as case 1'. > > Is it actually possible to get here with a wsize that > isn't 1,2,4 or 8, though? This function is used only > by the hmp 'x' and 'xp' commands. Those document that > the valid size specifications are b, h, w or g (for > 8, 16, 32 or 64 bits), and monitor_parse_arguments() > doesn't seem to have any undocumented handling that > would result in a different size value. And the > code in memory_dump() doesn't do anything sensible > with a wsize other than 1, 2, 4 or 8 -- if you hand > it a wsize of 3, for instance, I think it will print > every third byte. > > So I think that probably the default case here should > be g_assert_not_reached().
...better still, we could replace the whole switch(wsize) with "v = ldn_p(buf + i, wsize);". -- PMM
