Re: [Qemu-devel] [RFC] spapr: Use CamelCase properly

[Cédric Le Goater]

On 3/7/19 6:21 AM, David Gibson wrote:
> On Wed, Mar 06, 2019 at 08:22:51AM +0100, Cédric Le Goater wrote:
>> Hello,
>>
>> On 3/6/19 5:38 AM, David Gibson wrote:
>>> The qemu coding standard is to use CamelCase for type and structure names,
>>> and the pseries code follows that... sort of.  There are quite a lot of
>>> places where we bend the rules in order to preserve the capitalization of
>>> internal acronyms like "PHB", "TCE", "DIMM" and most commonly "sPAPR".
>>>
>>> That was a bad idea - it frequently leads to names ending up with
>>> unreadable clusters of capital letters, and means they don't appear to the
>>> eye as type identifiers, which is kind of the point of the CamelCase
>>> convention in the first place.
>>>
>>> In short, keeping type identifiers look like CamelCase is more important
>>> than preserving standard capitalization of internal "words".  So, this
>>> patch renames a heap of spapr internal type names to a more standard
>>> CamelCase.
>>
>> +1
>>  
>>> We also rename VIOsPAPR* to SpaprVio*, the less natural reverse ordering
>>> was only there in the first place to mitigate the capital-letter cluster
>>> of "sPAPRVIO".
>>
>> Could we remove the Device prefix ? I don't find it very useful.
> 
> [Aside: ITYM "suffix"]

arg. yes :)

> 
>>
>>      SpaprVioVtyDevice
>>      SpaprVioVlanDevice
> 
> Done.
> 
>>      SpaprVioDevice
> 
> I'll leave this one.  "SpaprVio" doesn't really make grammatical
> sense, and "SpaprVioDevice" works in analogy with, say, PCIDevice.

ok.

>>
>>> This is 100% a mechanical search-and-replace patch.  It will, however,
>>> conflict with essentially any and all outstanding patches touching the
>>> spapr code.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>
>> I have spotted dromedaries :
>>
>>      typedef struct SpaprDRConnector
> 
> I've changed this to SpaprDrc.  Shorter and makes it clearer that
> these are the same things as the "DRC"s we reference elsewhere.
> 
>>      typedef struct SpaprDRCPhysical
> 
> Changed to SpaprDrcPhysical
> 
>>
>> Could we get rid of the 'State' prefix ? It does not add much to the name
>> and usually the associated macros remove it.
> 
> True, but it is a common qemu pattern, so I think I'll leave these for now.

ok

>>      SpaprRngState
>>      SpaprPhbState
>>      SpaprRtcState
>>      SpaprDimmState
>>      SpaprMachineState
>>      SpaprCpuState
>>
>> It would be good to reverse the xics filenames also.
> 
> I'm not really sure what you mean by that, and in any case I don't
> think it's really in scope for this patch.
 
To be consistent with the other filenames, we should change :

        xics_spapr.h  ->  spapr_xics.h
        xics_spapr.c  ->  spapr_xics.c
        xics_kvm.c    ->  spapr_xics_kvm.c

and yes, it should be in another patch.

Thanks,

C.