[libvirt] [PATCH v3 0/4] qemu: fix type of default video device

[Pavel Mores]

On Mon, Nov 25, 2019 at 10:04:58AM -0500, Cole Robinson wrote:
> On 11/25/19 9:40 AM, Pavel Mores wrote:
> > On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
> >> On 11/25/19 5:54 AM, Pavel Mores wrote:
> >>> This new version mostly integrates Cole's comments about the second version.
> >>> Refactoring and behaviour change are now separate commits.  Tests succeed for
> >>> every individual patch in the series.
> >>>
> >>> Pavel Mores (4):
> >>>   qemu: default video device type selection algoritm moved into its own
> >>>     function
> >>>   qemu: prepare existing test for change of the default video device
> >>>     type
> >>>   qemu: the actual change of default video devide type selection
> >>>     algorithm
> >>>   qemu: added tests of the new default video type selection algorithm
> >>
> >> Reviewed-by: Cole Robinson <crobinso at redhat.com>
> >>
> >> and pushed now.
> >>
> >> patch #3 still had some style issues I pointed out in the previous
> >> review: using 'else' when it isn't necessary, because every block
> >> returns. Better to use the style of qemuDomainDefaultNetModel which
> >> saves having to do any nested logic. I'll be happy to review a follow up
> >> cleanup patch for that
> > 
> > Oh, now I see I changed my own additions but overlooked the style of
> > pre-existing code.  Sorry about that - I guess I don't have an eye for this as
> > personally I'd probably slightly prefer the else-based style.
> > 
> 
> The main issue with the code as implemented is that the third block is
> indented under an 'else' when it doesn't need to be. If we followed that
> pattern to the extreme the code would look like
> 
> if (condition1) {
>     return foo;
> } else {
>     if (condition2) {
>         return bar;
>     } else {
>         if (condition3)
>             return baz;
>     }
> }
> 
> instead of
> 
> if (condition1)
>     return foo;
> if (condition2)
>     return bar;
> if (condition3)
>     return baz;

I agree that the way the code is now, the flat style is more readable.  I just
feel that the additional structure provided by nesting could come handy if the
algorithm has be changed again in a way that made it impossible to have just a
return in every branch.

If we can reasonably assume that that's not going to happen, and if the
original author doesn't object (cc'ing him), I'm happy to make the change.

Thanks,

	pvl