Re: [libvirt] [PATCH 09/10] conf: Provide missing virDomain*DefNew() functions
On Fri, Jun 30, 2017 at 12:34:15PM +0200, Andrea Bolognani wrote:
On Fri, 2017-06-30 at 10:18 +0200, Ján Tomko wrote:
> > Same as virDomainDeviceInfo itself, any struct that
> > embeds it needs to be initialized properly before use;
> > however, none of the structs in question even had a
> > proper allocation function defined.
> >
> > Implement an allocation function for all structs
> > embedding a virDomainDeviceInfo and use them instead
> > of plain VIR_ALLOC() everywhere.
>
> NACK
>
> This is a pointless obfuscation
Would you mind spending a few words to explain why you feel
that's the case?
Having a function where you perform both allocation and
initialization of your data structure is a pretty common
As of now, nothing special is initialized in the structure.
If you are going to need initialize something in the future,
please mention it also in the commit message, not just the
cover letter. This makes the obfuscation pointful.
pattern both outside and inside libvirt, and while it adds
one layer of indirection it also improves flexibility and
encapsulation, which makes it IMHO well worth it.
I am not a fan of adding code just in case we might need it
in the future, which is what this patch looks like to a lazy
reviewer that does not read cover letters for cleanups/refactors.
Jan
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)