Peter Xu <pet...@redhat.com> writes:
> On Tue, Mar 22, 2016 at 12:32:28PM -0600, Eric Blake wrote:
>> On 03/17/2016 09:27 PM, Peter Xu wrote:
>> > +##
>> > +# @GICCapability:
>> > +#
>> > +# This struct describes capability for a specific GIC version. These
>>
>> Might be nice to spell out what the acronym GIC means, but that's cosmetic.
>
> Ah! I thought I have added that... It's missing again. Will do in
> next spin.
>
>>
>> > +# bits are not only decided by QEMU/KVM software version, but also
>> > +# decided by the hardware that the program is running upon.
>> > +#
>> > +# @version: version of GIC to be described.
>> > +#
>> > +# @emulated: whether current QEMU/hardware supports emulated GIC
>> > +# device in user space.
>> > +#
>> > +# @kernel: whether current QEMU/hardware supports hardware
>> > +# accelerated GIC device in kernel.
>> > +#
>> > +# Since: 2.6
>> > +##
>> > +{ 'struct': 'GICCapability',
>> > + 'data': { 'version': 'int',
>> > + 'emulated': 'bool',
>> > + 'kernel': 'bool' } }
>> >
>>
>> I might have squashed this with the patch that first uses GICCapability,
>> as defining a type in isolation doesn't do much.
>
> I can do the squash in next spin if you prefer that one. Actually I
> got this question before, about when I should split and when to
> squash. E.g., shall I make sure that I should have no "definition
> only" patches in the future?
Depends.
The general rule is to keep separate things separate, and patches
self-contained. The narrow sense of self-contained is each patch
compiles and works. The wider sense is each patch makes sense to its
readers on its own. You can't always have a perfect score on the
latter, but you should try.
Adding a definition without a user is generally not advised, because you
generally need to see the user to make sense of it.
For a complex feature, adding its abstract interface before its concrete
implementation may help liberate interface review from implementation
details.
Note that your interface consists of type GICCapability and command
query-gic-capabilities. You could add just the interface with a stub
implementation first, then flesh out the implementation. But I doubt
the thing is complex enough to justify that.
