| Message ID | 20200810105147.10670-1-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] CODING_STYLE.rst: flesh out our naming conventions. | expand |
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 > ===============
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).
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.
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'
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.
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
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 ===============
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(-)