Re: [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

Am 18.11.2015 um 17:26 hat Eric Blake geschrieben:
> On 11/18/2015 05:08 AM, Kevin Wolf wrote:
> > Am 18.11.2015 um 11:30 hat Markus Armbruster geschrieben:
> >> Eric Blake <eblake@redhat.com> writes:
> >>
> >>> No need to keep two separate enums, where editing one is likely
> >>> to forget the other.  Now that we can specify a qapi enum prefix,
> >>> we don't even have to change the bulk of the uses.
> >>>
> 
> >>>  ##
> >>> -{ 'enum': 'BlkdebugEvent',
> >>> +{ 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
> >>>    'data': [ 'l1_update', 'l1_grow.alloc_table', 'l1_grow.write_table',
> >>>              'l1_grow.activate_table', 'l2_load', 'l2_update',
> >>>              'l2_update_compressed', 'l2_alloc.cow_read', 'l2_alloc.write',
> >>
> >> I'm no friend of the 'prefix' feature.  You could avoid it here by
> >> renaming BlkdebugEvent to Blkdbg.  No additional churn, because you
> >> already replace hand-written BlkDebugEvent by generated BlkdebugEvent.
> >>
> >> Alternatively, you could reduce churn by renaming it to BlkDebugEvent.
> >>
> >> Matter of taste.  Perhaps Kevin has a preference.
> > 
> > Wouldn't that mean that we end up with a C type called Blkdbg? In my
> > opinion that's a bit unspecific, so if you ask me, I would paint my
> > bikeshed in a different colour.
> 
> Most of the existing qapi names are Blkdebug, it was only the internal
> enum being replaced that used BlkDebug.  Also, qapi would munge BlkDebug
> into BLK_DEBUG, so I think we want to stick with the lowercase 'd' so
> that the munging (without 'prefix') would stick to BLKDEBUG.

Oh, I don't really have an opinion whether BlockDebug, BlkDebug or
Blkdebug is the best spelling. I think Blkdebug makes sense.

But I do have a strong opinion on whether the enum should be called
BlkdebugEvent or just Blkdbg (without any mention of "Event"). The
latter doesn't describe any more what the type is about.

> I'm also fine if you want me to do a followup patch that renames all
> enums from BLKDBG_L1_UPDATE to BLKDEBUG_EVENT_L1_UPDATE, at which point
> we could drop the 'prefix' (I don't know how many lines it would make
> long enough to need different wrapping, but modulo wrapping it would all
> be a mechanically scripted change).

I don't mind the 'prefix' feature. I think it's nice to have short, but
still unique enough constant names, but if Markus is bothered enough to
send a patch, I can live with BLKDEBUG_EVENT_*.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]