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

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(a)redhat.com&gt; >> >> 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;