* [PATCH-for-6.2? 0/3] docs/devel/style: Improve rST rendering @ 2021-11-16 15:13 Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions " Philippe Mathieu-Daudé ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-16 15:13 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Daniel P . Berrange, Markus Armbruster, Philippe Mathieu-Daudé Various changes in docs/devel/style.rst to improve its rST rendering (around C types/qualifiers/functions). I split it in 3 patches to ease reviewing, but feel free to squash if it makes life easier. Philippe Mathieu-Daudé (3): docs/devel/style: Improve GLib functions rST rendering docs/devel/style: Improve Error** functions rST rendering docs/devel/style: Improve types/qualifiers rST rendering docs/devel/style.rst | 172 ++++++++++++++++++++++--------------------- 1 file changed, 87 insertions(+), 85 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions rST rendering 2021-11-16 15:13 [PATCH-for-6.2? 0/3] docs/devel/style: Improve rST rendering Philippe Mathieu-Daudé @ 2021-11-16 15:13 ` Philippe Mathieu-Daudé 2021-11-18 10:58 ` Darren Kenny 2021-11-16 15:13 ` [PATCH-for-6.2? 2/3] docs/devel/style: Improve Error** " Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 3/3] docs/devel/style: Improve types/qualifiers " Philippe Mathieu-Daudé 2 siblings, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-16 15:13 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Daniel P . Berrange, Markus Armbruster, Philippe Mathieu-Daudé Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- docs/devel/style.rst | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 260e3263fa0..415a6b9d700 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -413,13 +413,14 @@ multiple exist paths you can also improve the readability of the code by using ``g_autofree`` and related annotations. See :ref:`autofree-ref` for more details. -Calling ``g_malloc`` with a zero size is valid and will return NULL. +Calling ``g_malloc`` with a zero size is valid and will return ``NULL``. Prefer ``g_new(T, n)`` instead of ``g_malloc(sizeof(T) * n)`` for the following reasons: -* It catches multiplication overflowing size_t; -* It returns T ``*`` instead of void ``*``, letting compiler catch more type errors. +* It catches multiplication overflowing ``size_t``; +* It returns ``T *`` instead of ``void *``, letting compiler catch more type + errors. Declarations like @@ -444,14 +445,14 @@ use this similar function when possible, but note its different signature: void pstrcpy(char *dest, int dest_buf_size, const char *src) -Don't use strcat because it can't check for buffer overflows, but: +Don't use ``strcat`` because it can't check for buffer overflows, but: .. code-block:: c char *pstrcat(char *buf, int buf_size, const char *s) -The same limitation exists with sprintf and vsprintf, so use snprintf and -vsnprintf. +The same limitation exists with ``sprintf`` and ``vsprintf``, so use +``snprintf`` and ``vsnprintf``. QEMU provides other useful string functions: @@ -464,8 +465,8 @@ QEMU provides other useful string functions: There are also replacement character processing macros for isxyz and toxyz, so instead of e.g. isalnum you should use qemu_isalnum. -Because of the memory management rules, you must use g_strdup/g_strndup -instead of plain strdup/strndup. +Because of the memory management rules, you must use ``g_strdup/g_strndup`` +instead of plain ``strdup/strndup``. Printf-style functions ====================== @@ -524,10 +525,10 @@ automatic cleanup: Most notably: -* g_autofree - will invoke g_free() on the variable going out of scope +* ``g_autofree`` - will invoke ``g_free()`` on the variable going out of scope -* g_autoptr - for structs / objects, will invoke the cleanup func created - by a previous use of G_DEFINE_AUTOPTR_CLEANUP_FUNC. This is +* ``g_autoptr`` - for structs / objects, will invoke the cleanup func created + by a previous use of ``G_DEFINE_AUTOPTR_CLEANUP_FUNC``. This is supported for most GLib data types and GObjects For example, instead of @@ -551,7 +552,7 @@ For example, instead of return ret; } -Using g_autofree/g_autoptr enables the code to be written as: +Using ``g_autofree/g_autoptr`` enables the code to be written as: .. code-block:: c @@ -569,13 +570,13 @@ Using g_autofree/g_autoptr enables the code to be written as: While this generally results in simpler, less leak-prone code, there are still some caveats to beware of -* Variables declared with g_auto* MUST always be initialized, +* Variables declared with ``g_auto*`` MUST always be initialized, otherwise the cleanup function will use uninitialized stack memory -* If a variable declared with g_auto* holds a value which must +* If a variable declared with ``g_auto*`` holds a value which must live beyond the life of the function, that value must be saved and the original variable NULL'd out. This can be simpler using - g_steal_pointer + ``g_steal_pointer`` .. code-block:: c -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions rST rendering 2021-11-16 15:13 ` [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions " Philippe Mathieu-Daudé @ 2021-11-18 10:58 ` Darren Kenny 2021-11-18 12:12 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 8+ messages in thread From: Darren Kenny @ 2021-11-18 10:58 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Peter Maydell, Daniel P . Berrange, Markus Armbruster, Philippe Mathieu-Daudé Hi Philippe, There are some inconsistencies in the use of '()' when referring to functions or macros below... On Tuesday, 2021-11-16 at 16:13:15 +01, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > docs/devel/style.rst | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/docs/devel/style.rst b/docs/devel/style.rst > index 260e3263fa0..415a6b9d700 100644 > --- a/docs/devel/style.rst > +++ b/docs/devel/style.rst > @@ -413,13 +413,14 @@ multiple exist paths you can also improve the readability of the code > by using ``g_autofree`` and related annotations. See :ref:`autofree-ref` > for more details. > > -Calling ``g_malloc`` with a zero size is valid and will return NULL. > +Calling ``g_malloc`` with a zero size is valid and will return ``NULL``. > g_malloc() ? > > Prefer ``g_new(T, n)`` instead of ``g_malloc(sizeof(T) * n)`` for the following > reasons: > > -* It catches multiplication overflowing size_t; > -* It returns T ``*`` instead of void ``*``, letting compiler catch more type errors. > +* It catches multiplication overflowing ``size_t``; > +* It returns ``T *`` instead of ``void *``, letting compiler catch more type > + errors. > > Declarations like > > @@ -444,14 +445,14 @@ use this similar function when possible, but note its different signature: > > void pstrcpy(char *dest, int dest_buf_size, const char *src) > > -Don't use strcat because it can't check for buffer overflows, but: > +Don't use ``strcat`` because it can't check for buffer overflows, but: > strcat() ? > > .. code-block:: c > > char *pstrcat(char *buf, int buf_size, const char *s) > > -The same limitation exists with sprintf and vsprintf, so use snprintf and > -vsnprintf. > +The same limitation exists with ``sprintf`` and ``vsprintf``, so use sprintf() and vsprintf()? > +``snprintf`` and ``vsnprintf``. > snprintf() and vsnprintf()? > > QEMU provides other useful string functions: > > @@ -464,8 +465,8 @@ QEMU provides other useful string functions: > There are also replacement character processing macros for isxyz and toxyz, > so instead of e.g. isalnum you should use qemu_isalnum. > Should this be isalnum() and qemu_isalnum()? > > -Because of the memory management rules, you must use g_strdup/g_strndup > -instead of plain strdup/strndup. > +Because of the memory management rules, you must use ``g_strdup/g_strndup`` > Wonder should this be ``g_strdup()``/``g_strndup()`` > +instead of plain ``strdup/strndup``. > And ``strdup()``/``strndup()`` > > Printf-style functions > ====================== > @@ -524,10 +525,10 @@ automatic cleanup: > > Most notably: > > -* g_autofree - will invoke g_free() on the variable going out of scope > +* ``g_autofree`` - will invoke ``g_free()`` on the variable going out of scope > g_autofree() ? > > -* g_autoptr - for structs / objects, will invoke the cleanup func created > - by a previous use of G_DEFINE_AUTOPTR_CLEANUP_FUNC. This is > +* ``g_autoptr`` - for structs / objects, will invoke the cleanup func created > g_autoptr() ? > + by a previous use of ``G_DEFINE_AUTOPTR_CLEANUP_FUNC``. This is > supported for most GLib data types and GObjects > > For example, instead of > @@ -551,7 +552,7 @@ For example, instead of > return ret; > } > > -Using g_autofree/g_autoptr enables the code to be written as: > +Using ``g_autofree/g_autoptr`` enables the code to be written as: > ``g_autofree()``/``g_autoptr()`` ? > > .. code-block:: c > > @@ -569,13 +570,13 @@ Using g_autofree/g_autoptr enables the code to be written as: > While this generally results in simpler, less leak-prone code, there > are still some caveats to beware of > > -* Variables declared with g_auto* MUST always be initialized, > +* Variables declared with ``g_auto*`` MUST always be initialized, > g_auto*() ? > otherwise the cleanup function will use uninitialized stack memory > > -* If a variable declared with g_auto* holds a value which must > +* If a variable declared with ``g_auto*`` holds a value which must > g_auto*() ? > live beyond the life of the function, that value must be saved > and the original variable NULL'd out. This can be simpler using > - g_steal_pointer > + ``g_steal_pointer`` > g_steal_pointer() ? Thanks, Darren. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions rST rendering 2021-11-18 10:58 ` Darren Kenny @ 2021-11-18 12:12 ` Philippe Mathieu-Daudé 2021-11-18 13:03 ` Daniel P. Berrangé 0 siblings, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-18 12:12 UTC (permalink / raw) To: Daniel P . Berrange Cc: Darren Kenny, qemu-devel, Markus Armbruster, Peter Maydell On 11/18/21 11:58, Darren Kenny wrote: > Hi Philippe, > > There are some inconsistencies in the use of '()' when referring to > functions or macros below... Daniel, if you agree with Darren comments I can respin addressing them. > On Tuesday, 2021-11-16 at 16:13:15 +01, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> docs/devel/style.rst | 31 ++++++++++++++++--------------- >> 1 file changed, 16 insertions(+), 15 deletions(-) >> >> diff --git a/docs/devel/style.rst b/docs/devel/style.rst >> index 260e3263fa0..415a6b9d700 100644 >> --- a/docs/devel/style.rst >> +++ b/docs/devel/style.rst >> @@ -413,13 +413,14 @@ multiple exist paths you can also improve the readability of the code >> by using ``g_autofree`` and related annotations. See :ref:`autofree-ref` >> for more details. >> >> -Calling ``g_malloc`` with a zero size is valid and will return NULL. >> +Calling ``g_malloc`` with a zero size is valid and will return ``NULL``. >> > > g_malloc() ? > >> >> Prefer ``g_new(T, n)`` instead of ``g_malloc(sizeof(T) * n)`` for the following >> reasons: >> >> -* It catches multiplication overflowing size_t; >> -* It returns T ``*`` instead of void ``*``, letting compiler catch more type errors. >> +* It catches multiplication overflowing ``size_t``; >> +* It returns ``T *`` instead of ``void *``, letting compiler catch more type >> + errors. >> >> Declarations like >> >> @@ -444,14 +445,14 @@ use this similar function when possible, but note its different signature: >> >> void pstrcpy(char *dest, int dest_buf_size, const char *src) >> >> -Don't use strcat because it can't check for buffer overflows, but: >> +Don't use ``strcat`` because it can't check for buffer overflows, but: >> > > strcat() ? > >> >> .. code-block:: c >> >> char *pstrcat(char *buf, int buf_size, const char *s) >> >> -The same limitation exists with sprintf and vsprintf, so use snprintf and >> -vsnprintf. >> +The same limitation exists with ``sprintf`` and ``vsprintf``, so use > > sprintf() and vsprintf()? > >> +``snprintf`` and ``vsnprintf``. >> > > snprintf() and vsnprintf()? > >> >> QEMU provides other useful string functions: >> >> @@ -464,8 +465,8 @@ QEMU provides other useful string functions: >> There are also replacement character processing macros for isxyz and toxyz, >> so instead of e.g. isalnum you should use qemu_isalnum. >> > > Should this be isalnum() and qemu_isalnum()? > >> >> -Because of the memory management rules, you must use g_strdup/g_strndup >> -instead of plain strdup/strndup. >> +Because of the memory management rules, you must use ``g_strdup/g_strndup`` >> > > Wonder should this be ``g_strdup()``/``g_strndup()`` > >> +instead of plain ``strdup/strndup``. >> > > And ``strdup()``/``strndup()`` > >> >> Printf-style functions >> ====================== >> @@ -524,10 +525,10 @@ automatic cleanup: >> >> Most notably: >> >> -* g_autofree - will invoke g_free() on the variable going out of scope >> +* ``g_autofree`` - will invoke ``g_free()`` on the variable going out of scope >> > > g_autofree() ? > >> >> -* g_autoptr - for structs / objects, will invoke the cleanup func created >> - by a previous use of G_DEFINE_AUTOPTR_CLEANUP_FUNC. This is >> +* ``g_autoptr`` - for structs / objects, will invoke the cleanup func created >> > > g_autoptr() ? > >> + by a previous use of ``G_DEFINE_AUTOPTR_CLEANUP_FUNC``. This is >> supported for most GLib data types and GObjects >> >> For example, instead of >> @@ -551,7 +552,7 @@ For example, instead of >> return ret; >> } >> >> -Using g_autofree/g_autoptr enables the code to be written as: >> +Using ``g_autofree/g_autoptr`` enables the code to be written as: >> > > ``g_autofree()``/``g_autoptr()`` ? > >> >> .. code-block:: c >> >> @@ -569,13 +570,13 @@ Using g_autofree/g_autoptr enables the code to be written as: >> While this generally results in simpler, less leak-prone code, there >> are still some caveats to beware of >> >> -* Variables declared with g_auto* MUST always be initialized, >> +* Variables declared with ``g_auto*`` MUST always be initialized, >> > > g_auto*() ? > >> otherwise the cleanup function will use uninitialized stack memory >> >> -* If a variable declared with g_auto* holds a value which must >> +* If a variable declared with ``g_auto*`` holds a value which must >> > > g_auto*() ? > >> live beyond the life of the function, that value must be saved >> and the original variable NULL'd out. This can be simpler using >> - g_steal_pointer >> + ``g_steal_pointer`` >> > > g_steal_pointer() ? > > Thanks, > > Darren. > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions rST rendering 2021-11-18 12:12 ` Philippe Mathieu-Daudé @ 2021-11-18 13:03 ` Daniel P. Berrangé 0 siblings, 0 replies; 8+ messages in thread From: Daniel P. Berrangé @ 2021-11-18 13:03 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Darren Kenny, qemu-devel, Markus Armbruster, Peter Maydell On Thu, Nov 18, 2021 at 01:12:26PM +0100, Philippe Mathieu-Daudé wrote: > On 11/18/21 11:58, Darren Kenny wrote: > > Hi Philippe, > > > > There are some inconsistencies in the use of '()' when referring to > > functions or macros below... > > Daniel, if you agree with Darren comments I can respin addressing them. It is fine with me. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH-for-6.2? 2/3] docs/devel/style: Improve Error** functions rST rendering 2021-11-16 15:13 [PATCH-for-6.2? 0/3] docs/devel/style: Improve rST rendering Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions " Philippe Mathieu-Daudé @ 2021-11-16 15:13 ` Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 3/3] docs/devel/style: Improve types/qualifiers " Philippe Mathieu-Daudé 2 siblings, 0 replies; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-16 15:13 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Daniel P . Berrange, Markus Armbruster, Philippe Mathieu-Daudé Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- docs/devel/style.rst | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 415a6b9d700..21f0f213193 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -602,16 +602,16 @@ Error handling and reporting Reporting errors to the human user ---------------------------------- -Do not use printf(), fprintf() or monitor_printf(). Instead, use -error_report() or error_vreport() from error-report.h. This ensures the -error is reported in the right place (current monitor or stderr), and in -a uniform format. +Do not use ``printf()``, ``fprintf()`` or ``monitor_printf()``. Instead, use +``error_report()`` or ``error_vreport()`` from error-report.h. This ensures +the error is reported in the right place (current monitor or ``stderr``), and +in a uniform format. -Use error_printf() & friends to print additional information. +Use ``error_printf()`` & friends to print additional information. -error_report() prints the current location. In certain common cases +``error_report()`` prints the current location. In certain common cases like command line parsing, the current location is tracked -automatically. To manipulate it manually, use the loc_``*``() from +automatically. To manipulate it manually, use the ``loc_*()`` from error-report.h. Propagating errors @@ -621,7 +621,7 @@ An error can't always be reported to the user right where it's detected, but often needs to be propagated up the call chain to a place that can handle it. This can be done in various ways. -The most flexible one is Error objects. See error.h for usage +The most flexible one is ``Error`` objects. See error.h for usage information. Use the simplest suitable method to communicate success / failure to @@ -631,10 +631,10 @@ error, non-negative / -errno, non-null / null, or Error objects. Example: when a function returns a non-null pointer on success, and it can fail only in one way (as far as the caller is concerned), returning null on failure is just fine, and certainly simpler and a lot easier on -the eyes than propagating an Error object through an Error ``*````*`` parameter. +the eyes than propagating an Error object through an ``Error **`` parameter. Example: when a function's callers need to report details on failure -only the function really knows, use Error ``*````*``, and set suitable errors. +only the function really knows, use ``Error **``, and set suitable errors. Do not report an error to the user when you're also returning an error for somebody else to handle. Leave the reporting to the place that @@ -643,17 +643,17 @@ consumes the error returned. Handling errors --------------- -Calling exit() is fine when handling configuration errors during +Calling ``exit()`` is fine when handling configuration errors during startup. It's problematic during normal operation. In particular, -monitor commands should never exit(). +monitor commands should never ``exit()``. -Do not call exit() or abort() to handle an error that can be triggered +Do not call ``exit()`` or ``abort()`` to handle an error that can be triggered by the guest (e.g., some unimplemented corner case in guest code translation or device emulation). Guests should not be able to terminate QEMU. -Note that &error_fatal is just another way to exit(1), and &error_abort -is just another way to abort(). +Note that ``&error_fatal`` is just another way to ``exit(1)``, and +``&error_abort`` is just another way to ``abort()``. trace-events style -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH-for-6.2? 3/3] docs/devel/style: Improve types/qualifiers rST rendering 2021-11-16 15:13 [PATCH-for-6.2? 0/3] docs/devel/style: Improve rST rendering Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions " Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 2/3] docs/devel/style: Improve Error** " Philippe Mathieu-Daudé @ 2021-11-16 15:13 ` Philippe Mathieu-Daudé 2021-11-18 11:04 ` Darren Kenny 2 siblings, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-16 15:13 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Daniel P . Berrange, Markus Armbruster, Philippe Mathieu-Daudé Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- docs/devel/style.rst | 111 ++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 21f0f213193..f9f063ed8cb 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -111,7 +111,7 @@ Variables are lower_case_with_underscores; easy to type and read. Structured type names are in CamelCase; harder to type but standing out. Enum type names and function type names should also be in CamelCase. Scalar type names are lower_case_with_underscores_ending_with_a_t, like the POSIX -uint64_t and family. Note that this last convention contradicts POSIX +``uint64_t`` and family. Note that this last convention contradicts POSIX and is therefore likely to be changed. Variable Naming Conventions @@ -195,9 +195,9 @@ blocks) are generally not allowed; declarations should be at the beginning of blocks. Every now and then, an exception is made for declarations inside a -#ifdef or #ifndef block: if the code looks nicer, such declarations can +``#ifdef`` or ``#ifndef`` block: if the code looks nicer, such declarations can be placed at the top of the block even if there are statements above. -On the other hand, however, it's often best to move that #ifdef/#ifndef +On the other hand, however, it's often best to move that ``#ifdef/#ifndef`` block to a separate function altogether. Conditional statements @@ -220,13 +220,13 @@ even when the constant is on the right. Comment style ============= -We use traditional C-style /``*`` ``*``/ comments and avoid // comments. +We use traditional C-style ``/*`` ``*/`` comments and avoid ``//`` comments. -Rationale: The // form is valid in C99, so this is purely a matter of +Rationale: The ``//`` form is valid in C99, so this is purely a matter of consistency of style. The checkpatch script will warn you about this. Multiline comment blocks should have a row of stars on the left, -and the initial /``*`` and terminating ``*``/ both on their own lines: +and the initial ``/*`` and terminating ``*/`` both on their own lines: .. code-block:: c @@ -290,57 +290,57 @@ a few useful guidelines here. Scalars ------- -If you're using "int" or "long", odds are good that there's a better type. -If a variable is counting something, it should be declared with an -unsigned type. +If you're using '``int``' or '``long``', odds are good that there's a better +type. If a variable is counting something, it should be declared with an +*unsigned* type. -If it's host memory-size related, size_t should be a good choice (use -ssize_t only if required). Guest RAM memory offsets must use ram_addr_t, +If it's host memory-size related, ``size_t`` should be a good choice (use +``ssize_t`` only if required). Guest RAM memory offsets must use ``ram_addr_t``, but only for RAM, it may not cover whole guest address space. -If it's file-size related, use off_t. -If it's file-offset related (i.e., signed), use off_t. -If it's just counting small numbers use "unsigned int"; +If it's file-size related, use ``off_t``. +If it's file-offset related (i.e., signed), use ``off_t``. +If it's just counting small numbers use '``unsigned int``'; (on all but oddball embedded systems, you can assume that that type is at least four bytes wide). In the event that you require a specific width, use a standard type -like int32_t, uint32_t, uint64_t, etc. The specific types are +like ``int32_t``, ``uint32_t``, ``uint64_t``, etc. The specific types are mandatory for VMState fields. -Don't use Linux kernel internal types like u32, __u32 or __le32. +Don't use Linux kernel internal types like ``u32``, ``__u32`` or ``__le32``. -Use hwaddr for guest physical addresses except pcibus_t -for PCI addresses. In addition, ram_addr_t is a QEMU internal address +Use ``hwaddr`` for guest physical addresses except ``pcibus_t`` +for PCI addresses. In addition, ``ram_addr_t`` is a QEMU internal address space that maps guest RAM physical addresses into an intermediate address space that can map to host virtual address spaces. Generally -speaking, the size of guest memory can always fit into ram_addr_t but +speaking, the size of guest memory can always fit into ``ram_addr_t`` but it would not be correct to store an actual guest physical address in a -ram_addr_t. +``ram_addr_t``. For CPU virtual addresses there are several possible types. -vaddr is the best type to use to hold a CPU virtual address in +``vaddr`` is the best type to use to hold a CPU virtual address in target-independent code. It is guaranteed to be large enough to hold a virtual address for any target, and it does not change size from target to target. It is always unsigned. -target_ulong is a type the size of a virtual address on the CPU; this means +``target_ulong`` is a type the size of a virtual address on the CPU; this means it may be 32 or 64 bits depending on which target is being built. It should therefore be used only in target-specific code, and in some performance-critical built-per-target core code such as the TLB code. -There is also a signed version, target_long. -abi_ulong is for the ``*``-user targets, and represents a type the size of -'void ``*``' in that target's ABI. (This may not be the same as the size of a +There is also a signed version, ``target_long``. +``abi_ulong`` is for the ``*-user`` targets, and represents a type the size of +'``void *``' in that target's ABI. (This may not be the same as the size of a full CPU virtual address in the case of target ABIs which use 32 bit pointers -on 64 bit CPUs, like sparc32plus.) Definitions of structures that must match +on 64 bit CPUs, like *sparc32plus*.) Definitions of structures that must match the target's ABI must use this type for anything that on the target is defined -to be an 'unsigned long' or a pointer type. -There is also a signed version, abi_long. +to be an '``unsigned long``' or a pointer type. +There is also a signed version, ``abi_long``. Of course, take all of the above with a grain of salt. If you're about -to use some system interface that requires a type like size_t, pid_t or -off_t, use matching types for any corresponding variables. +to use some system interface that requires a type like ``size_t``, ``pid_t`` or +``off_t``, use matching types for any corresponding variables. -Also, if you try to use e.g., "unsigned int" as a type, and that +Also, if you try to use e.g., '``unsigned int``' as a type, and that conflicts with the signedness of a related variable, sometimes it's best just to use the *wrong* type, if "pulling the thread" and fixing all related variables would be too invasive. @@ -352,9 +352,9 @@ casts, then reconsider or ask for help. Pointers -------- -Ensure that all of your pointers are "const-correct". +Ensure that all of your pointers are "``const``-correct". Unless a pointer is used to modify the pointed-to storage, -give it the "const" attribute. That way, the reader knows +give it the '``const``' attribute. That way, the reader knows up-front that this is a read-only pointer. Perhaps more importantly, if we're diligent about this, when you see a non-const pointer, you're guaranteed that it is used to modify the storage @@ -363,7 +363,7 @@ it points to, or it is aliased to another pointer that is. Typedefs -------- -Typedefs are used to eliminate the redundant 'struct' keyword, since type +Typedefs are used to eliminate the redundant '``struct``' keyword, since type names have a different style than other identifiers ("CamelCase" versus "snake_case"). Each named struct type should have a CamelCase name and a corresponding typedef. @@ -462,8 +462,8 @@ QEMU provides other useful string functions: int stristart(const char *str, const char *val, const char **ptr) int qemu_strnlen(const char *s, int max_len) -There are also replacement character processing macros for isxyz and toxyz, -so instead of e.g. isalnum you should use qemu_isalnum. +There are also replacement character processing macros for ``isxyz`` and +``toxyz``, so instead of e.g. ``isalnum`` you should use ``qemu_isalnum``. Because of the memory management rules, you must use ``g_strdup/g_strndup`` instead of plain ``strdup/strndup``. @@ -472,10 +472,10 @@ Printf-style functions ====================== Whenever you add a new printf-style function, i.e., one with a format -string argument and following "..." in its prototype, be sure to use +string argument and following '``...``' in its prototype, be sure to use gcc's printf attribute directive in the prototype. -This makes it so gcc's -Wformat and -Wformat-security options can do +This makes it so gcc's ``-Wformat`` and ``-Wformat-security`` options can do their jobs and cross-check format strings with the number and types of arguments. @@ -503,7 +503,7 @@ painful. These are: the sign bit (ie it is an arithmetic shift, not a logical shift) In addition, QEMU assumes that the compiler does not use the latitude -given in C99 and C11 to treat aspects of signed '<<' as undefined, as +given in C99 and C11 to treat aspects of signed '``<<``' as undefined, as documented in the GNU Compiler Collection manual starting at version 4.0. .. _autofree-ref: @@ -659,10 +659,10 @@ Note that ``&error_fatal`` is just another way to ``exit(1)``, and trace-events style ================== -0x prefix ---------- +``0x`` prefix +------------- -In trace-events files, use a '0x' prefix to specify hex numbers, as in: +In trace-events files, use a '``0x``' prefix to specify hex numbers, as in: .. code-block:: c @@ -676,27 +676,28 @@ PCI bus id): another_trace(int cssid, int ssid, int dev_num) "bus id: %x.%x.%04x" -However, you can use '0x' for such groups if you want. Anyway, be sure that +However, you can use '``0x``' for such groups if you want. Anyway, be sure that it is obvious that numbers are in hex, ex.: .. code-block:: c data_dump(uint8_t c1, uint8_t c2, uint8_t c3) "bytes (in hex): %02x %02x %02x" -Rationale: hex numbers are hard to read in logs when there is no 0x prefix, -especially when (occasionally) the representation doesn't contain any letters -and especially in one line with other decimal numbers. Number groups are allowed -to not use '0x' because for some things notations like %x.%x.%x are used not -only in Qemu. Also dumping raw data bytes with '0x' is less readable. +Rationale: hex numbers are hard to read in logs when there is no '``0x``' +prefix, especially when (occasionally) the representation doesn't contain any +letters and especially in one line with other decimal numbers. Number groups +are allowed to not use '``0x``' because for some things notations like +'``%x.%x.%x``' are used not only in QEMU. Also dumping raw data bytes with +'``0x``' is less readable. -'#' printf flag ---------------- +'``#``' printf flag +------------------- -Do not use printf flag '#', like '%#x'. +Do not use printf flag '``#``', like '``%#x``'. -Rationale: there are two ways to add a '0x' prefix to printed number: '0x%...' -and '%#...'. For consistency the only one way should be used. Arguments for -'0x%' are: +Rationale: there are two ways to add a '``0x``' prefix to printed number: +'``0x%...``' and '``%#...``'. For consistency the only one way should be used. +Arguments for '``0x%``' are: * it is more popular -* '%#' omits the 0x for the value 0 which makes output inconsistent +* '``%#``' omits the ``0x`` for the value ``0`` which makes output inconsistent -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH-for-6.2? 3/3] docs/devel/style: Improve types/qualifiers rST rendering 2021-11-16 15:13 ` [PATCH-for-6.2? 3/3] docs/devel/style: Improve types/qualifiers " Philippe Mathieu-Daudé @ 2021-11-18 11:04 ` Darren Kenny 0 siblings, 0 replies; 8+ messages in thread From: Darren Kenny @ 2021-11-18 11:04 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Peter Maydell, Daniel P . Berrange, Markus Armbruster, Philippe Mathieu-Daudé Hi Philippe, A couple here too w.r.t. function/macros... On Tuesday, 2021-11-16 at 16:13:17 +01, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > docs/devel/style.rst | 111 ++++++++++++++++++++++--------------------- > 1 file changed, 56 insertions(+), 55 deletions(-) > > diff --git a/docs/devel/style.rst b/docs/devel/style.rst > index 21f0f213193..f9f063ed8cb 100644 > --- a/docs/devel/style.rst > +++ b/docs/devel/style.rst > @@ -111,7 +111,7 @@ Variables are lower_case_with_underscores; easy to type and read. Structured > type names are in CamelCase; harder to type but standing out. Enum type > names and function type names should also be in CamelCase. Scalar type > names are lower_case_with_underscores_ending_with_a_t, like the POSIX > -uint64_t and family. Note that this last convention contradicts POSIX > +``uint64_t`` and family. Note that this last convention contradicts POSIX > and is therefore likely to be changed. > > Variable Naming Conventions > @@ -195,9 +195,9 @@ blocks) are generally not allowed; declarations should be at the beginning > of blocks. > > Every now and then, an exception is made for declarations inside a > -#ifdef or #ifndef block: if the code looks nicer, such declarations can > +``#ifdef`` or ``#ifndef`` block: if the code looks nicer, such declarations can > be placed at the top of the block even if there are statements above. > -On the other hand, however, it's often best to move that #ifdef/#ifndef > +On the other hand, however, it's often best to move that ``#ifdef/#ifndef`` > block to a separate function altogether. > > Conditional statements > @@ -220,13 +220,13 @@ even when the constant is on the right. > Comment style > ============= > > -We use traditional C-style /``*`` ``*``/ comments and avoid // comments. > +We use traditional C-style ``/*`` ``*/`` comments and avoid ``//`` comments. > > -Rationale: The // form is valid in C99, so this is purely a matter of > +Rationale: The ``//`` form is valid in C99, so this is purely a matter of > consistency of style. The checkpatch script will warn you about this. > > Multiline comment blocks should have a row of stars on the left, > -and the initial /``*`` and terminating ``*``/ both on their own lines: > +and the initial ``/*`` and terminating ``*/`` both on their own lines: > > .. code-block:: c > > @@ -290,57 +290,57 @@ a few useful guidelines here. > Scalars > ------- > > -If you're using "int" or "long", odds are good that there's a better type. > -If a variable is counting something, it should be declared with an > -unsigned type. > +If you're using '``int``' or '``long``', odds are good that there's a better > +type. If a variable is counting something, it should be declared with an > +*unsigned* type. > > -If it's host memory-size related, size_t should be a good choice (use > -ssize_t only if required). Guest RAM memory offsets must use ram_addr_t, > +If it's host memory-size related, ``size_t`` should be a good choice (use > +``ssize_t`` only if required). Guest RAM memory offsets must use ``ram_addr_t``, > but only for RAM, it may not cover whole guest address space. > > -If it's file-size related, use off_t. > -If it's file-offset related (i.e., signed), use off_t. > -If it's just counting small numbers use "unsigned int"; > +If it's file-size related, use ``off_t``. > +If it's file-offset related (i.e., signed), use ``off_t``. > +If it's just counting small numbers use '``unsigned int``'; > (on all but oddball embedded systems, you can assume that that > type is at least four bytes wide). > > In the event that you require a specific width, use a standard type > -like int32_t, uint32_t, uint64_t, etc. The specific types are > +like ``int32_t``, ``uint32_t``, ``uint64_t``, etc. The specific types are > mandatory for VMState fields. > > -Don't use Linux kernel internal types like u32, __u32 or __le32. > +Don't use Linux kernel internal types like ``u32``, ``__u32`` or ``__le32``. > > -Use hwaddr for guest physical addresses except pcibus_t > -for PCI addresses. In addition, ram_addr_t is a QEMU internal address > +Use ``hwaddr`` for guest physical addresses except ``pcibus_t`` > +for PCI addresses. In addition, ``ram_addr_t`` is a QEMU internal address > space that maps guest RAM physical addresses into an intermediate > address space that can map to host virtual address spaces. Generally > -speaking, the size of guest memory can always fit into ram_addr_t but > +speaking, the size of guest memory can always fit into ``ram_addr_t`` but > it would not be correct to store an actual guest physical address in a > -ram_addr_t. > +``ram_addr_t``. > > For CPU virtual addresses there are several possible types. > -vaddr is the best type to use to hold a CPU virtual address in > +``vaddr`` is the best type to use to hold a CPU virtual address in > target-independent code. It is guaranteed to be large enough to hold a > virtual address for any target, and it does not change size from target > to target. It is always unsigned. > -target_ulong is a type the size of a virtual address on the CPU; this means > +``target_ulong`` is a type the size of a virtual address on the CPU; this means > it may be 32 or 64 bits depending on which target is being built. It should > therefore be used only in target-specific code, and in some > performance-critical built-per-target core code such as the TLB code. > -There is also a signed version, target_long. > -abi_ulong is for the ``*``-user targets, and represents a type the size of > -'void ``*``' in that target's ABI. (This may not be the same as the size of a > +There is also a signed version, ``target_long``. > +``abi_ulong`` is for the ``*-user`` targets, and represents a type the size of > +'``void *``' in that target's ABI. (This may not be the same as the size of a > full CPU virtual address in the case of target ABIs which use 32 bit pointers > -on 64 bit CPUs, like sparc32plus.) Definitions of structures that must match > +on 64 bit CPUs, like *sparc32plus*.) Definitions of structures that must match > the target's ABI must use this type for anything that on the target is defined > -to be an 'unsigned long' or a pointer type. > -There is also a signed version, abi_long. > +to be an '``unsigned long``' or a pointer type. > +There is also a signed version, ``abi_long``. > > Of course, take all of the above with a grain of salt. If you're about > -to use some system interface that requires a type like size_t, pid_t or > -off_t, use matching types for any corresponding variables. > +to use some system interface that requires a type like ``size_t``, ``pid_t`` or > +``off_t``, use matching types for any corresponding variables. > > -Also, if you try to use e.g., "unsigned int" as a type, and that > +Also, if you try to use e.g., '``unsigned int``' as a type, and that > conflicts with the signedness of a related variable, sometimes > it's best just to use the *wrong* type, if "pulling the thread" > and fixing all related variables would be too invasive. > @@ -352,9 +352,9 @@ casts, then reconsider or ask for help. > Pointers > -------- > > -Ensure that all of your pointers are "const-correct". > +Ensure that all of your pointers are "``const``-correct". > Unless a pointer is used to modify the pointed-to storage, > -give it the "const" attribute. That way, the reader knows > +give it the '``const``' attribute. That way, the reader knows > up-front that this is a read-only pointer. Perhaps more > importantly, if we're diligent about this, when you see a non-const > pointer, you're guaranteed that it is used to modify the storage > @@ -363,7 +363,7 @@ it points to, or it is aliased to another pointer that is. > Typedefs > -------- > > -Typedefs are used to eliminate the redundant 'struct' keyword, since type > +Typedefs are used to eliminate the redundant '``struct``' keyword, since type > names have a different style than other identifiers ("CamelCase" versus > "snake_case"). Each named struct type should have a CamelCase name and a > corresponding typedef. > @@ -462,8 +462,8 @@ QEMU provides other useful string functions: > int stristart(const char *str, const char *val, const char **ptr) > int qemu_strnlen(const char *s, int max_len) > > -There are also replacement character processing macros for isxyz and toxyz, > -so instead of e.g. isalnum you should use qemu_isalnum. > +There are also replacement character processing macros for ``isxyz`` and > +``toxyz``, so instead of e.g. ``isalnum`` you should use ``qemu_isalnum``. > (Looks like a repeat of a change in patch 1, but possibly a different location) isalnum() and qemu_isalnum()? Thanks, Darren. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-18 13:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-16 15:13 [PATCH-for-6.2? 0/3] docs/devel/style: Improve rST rendering Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 1/3] docs/devel/style: Improve GLib functions " Philippe Mathieu-Daudé 2021-11-18 10:58 ` Darren Kenny 2021-11-18 12:12 ` Philippe Mathieu-Daudé 2021-11-18 13:03 ` Daniel P. Berrangé 2021-11-16 15:13 ` [PATCH-for-6.2? 2/3] docs/devel/style: Improve Error** " Philippe Mathieu-Daudé 2021-11-16 15:13 ` [PATCH-for-6.2? 3/3] docs/devel/style: Improve types/qualifiers " Philippe Mathieu-Daudé 2021-11-18 11:04 ` Darren Kenny
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.