Re: [PATCH v2] CODING_STYLE.rst: flesh out our naming conventions.

[Alex Bennée]

Cornelia Huck <cohuck@redhat.com> writes:

> On Mon, 10 Aug 2020 11:51:47 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
>
>> Mention a few of the more common naming conventions we follow in the
>> code base including common variable names and function prefix and
>> suffix examples.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> ---
>> v2
>>   - punctuation fixes suggested by Cornelia
>>   - re-worded section on qemu_ prefix
>>   - expanded on _locked suffix
>> ---
>>  CODING_STYLE.rst | 30 ++++++++++++++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/CODING_STYLE.rst b/CODING_STYLE.rst
>> index 427699e0e42..e7ae44aed7f 100644
>> --- a/CODING_STYLE.rst
>> +++ b/CODING_STYLE.rst
>> @@ -109,8 +109,34 @@ names are lower_case_with_underscores_ending_with_a_t, 
>> like the POSIX
>>  uint64_t and family.  Note that this last convention contradicts POSIX
>>  and is therefore likely to be changed.
>>  
>> -When wrapping standard library functions, use the prefix ``qemu_`` to alert
>> -readers that they are seeing a wrapped version; otherwise avoid this prefix.
>> +Variable Naming Conventions
>> +---------------------------
>> +
>> +A number of short naming conventions exist for variables that use
>> +common QEMU types. For example, the architecture independent CPUState
>> +this is often held as a ``cs`` pointer variable, whereas the concrete
>
> s/this//
>
>> +CPUArchState us usually held in a pointer called ``env``.
>
> s/us/is/
>
>> +
>> +Likewise, in device emulation code the common DeviceState is usually
>> +called ``dev`` with the actual status structure often uses the terse
>
> s/with/while/

Oops sorry about those - serves me right for trying to re-spin too quickly.

>
>> +``s`` or maybe ``foodev``.
>> +
>> +Function Naming Conventions
>> +---------------------------
>> +
>> +The ``qemu_`` prefix is used for utility functions that are widely
>> +called from across the code-base. This includes wrapped versions of
>> +standard library functions (e.g. qemu_strtol) where the prefix is
>> +added to the function name to alert readers that they are seeing a
>> +wrapped version; otherwise avoid this prefix.
>
> Hm... not so sure about "otherwise avoid this prefix". It sounds a bit
> like you should avoid it for anything but wrappers, but I think what we
> want to say is that qemu_ should be used for anything that is
> potentially useful in many places, but probably not if there is a
> better prefix?

Yeah it's a hangover from the previous phrasing. Our current usage
certainly isn't just for wrapped functions - qemu_mutex_lock_iothread and
friends for example are very specifically qemu utility functions rather
than wrapped functions.

We also have a bunch of static functions that should really not have the
prefix - qemu_kvm_start_vcpu for example looses nothing by just being
kvm_start_vcpu.

We also have functions that could arguably just use a subsystem prefix -
for example qemu_chr_fe_accept_input is very much a thing you only call
when dealing with chardev frontends (chr_fe).

I'm certainly not proposing mass renames but it's clear our usage is
wider than just wrapped functions.

If I re-arrange slightly we can roll from qemu_ to public functions:

  Function Naming Conventions
  ---------------------------

  The ``qemu_`` prefix is used for utility functions that are widely
  called from across the code-base. This includes wrapped versions of
  standard library functions (e.g. ``qemu_strtol``) where the prefix is
  added to the library function name to alert readers that they are
  seeing a wrapped version.

  Public functions from a file or subsystem (declared in headers) tend
  to have a consistent prefix to show where they came from. For example,
  ``tlb_`` for functions from ``cputlb.c`` or ``cpu_`` for functions
  from cpus.c.

  If there are two versions of a function to be called with or without a
  lock held, the function that expects the lock to be already held
  usually uses the suffix ``_locked``.

What do you think?

(note to self, _impl seems like another convention we should document at
some point).

-- 
Alex Bennée