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

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

[Alex Bennée]

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
+CPUArchState us usually held in a pointer called ``env``.
+
+Likewise, in device emulation code the common DeviceState is usually
+called ``dev`` with the actual status structure often uses the terse
+``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.
+
+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``.
+
+Public functions (i.e. declared in public headers) tend to be prefixed
+with the subsystem or file they came from. For example, ``tlb_`` for
+functions from ``cputlb.c`` or ``cpu_`` for functions from cpus.c.
 
 Block structure
 ===============
-- 
2.20.1

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

[Cornelia Huck]

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/

> +``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?

> +
> +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``.
> +
> +Public functions (i.e. declared in public headers) tend to be prefixed
> +with the subsystem or file they came from. For example, ``tlb_`` for
> +functions from ``cputlb.c`` or ``cpu_`` for functions from cpus.c.
>  
>  Block structure
>  ===============

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

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

[Cornelia Huck]

On Tue, 11 Aug 2020 12:48:38 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> 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?

There naturally are places that don't follow the convention (for
example, hw/intc/s390_flic.c is using the qemu_ prefix to mark the
non-kvm functions), but this makes sense for new code. Looks good to me.

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

[Philippe Mathieu-Daudé]

Hi Alex,

On 8/10/20 12:51 PM, Alex Bennée 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>
> 
> ---
...
> +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.
> +
> +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``.

And if there is only one version? I'm looking at:

  /* With q->lock */
  static void nvme_kick(NVMeQueuePair *q)
  {
  ...
  }

Should the style be enforced here and this function renamed
nvme_kick_locked()?

In this particular case, I think so, because we also have:

  /* With q->lock */
  static void nvme_put_free_req_locked(...)
  {
  ...
  }

  /* With q->lock */
  static void nvme_wake_free_req_locked(NVMeQueuePair *q)
  {
  ...
  }

For more cases:

$ git grep -A1 -i '\/\*.*with.*lock'

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

[Cornelia Huck]

On Tue, 11 Aug 2020 17:55:08 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Alex,
> 
> On 8/10/20 12:51 PM, Alex Bennée 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>
> > 
> > ---  
> ...
> > +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.
> > +
> > +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``.  
> 
> And if there is only one version? I'm looking at:
> 
>   /* With q->lock */
>   static void nvme_kick(NVMeQueuePair *q)
>   {
>   ...
>   }
> 
> Should the style be enforced here and this function renamed
> nvme_kick_locked()?
> 
> In this particular case, I think so, because we also have:
> 
>   /* With q->lock */
>   static void nvme_put_free_req_locked(...)
>   {
>   ...
>   }
> 
>   /* With q->lock */
>   static void nvme_wake_free_req_locked(NVMeQueuePair *q)
>   {
>   ...
>   }
> 
> For more cases:
> 
> $ git grep -A1 -i '\/\*.*with.*lock'
> 
> 

I'm not sure we really want to encode calling conventions into function
names, beyond being able to distinguish between lock/no-lock versions.
Just appending _locked does not really tell us *which* lock is supposed
to be held, that needs to be documented in a comment anyway.

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

[Thomas Huth]

On 10/08/2020 12.51, Alex Bennée 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
> +CPUArchState us usually held in a pointer called ``env``.
> +
> +Likewise, in device emulation code the common DeviceState is usually
> +called ``dev`` with the actual status structure often uses the terse
> +``s`` or maybe ``foodev``.

Please let's not recommend single-letter variables like 's' here. This 
is a really bad idea, since they are hard to "grep" and can be confused 
quite easily. I think it would be best to simply drop that last part of 
the sentence (starting with "with the actual...").

  Thomas