[libvirt PATCH 01/10] docs: Convert hacking.html to reStructuredText
Daniel P. Berrangé
berrange at redhat.com
Tue Apr 7 18:02:09 UTC 2020
On Mon, Apr 06, 2020 at 06:20:01PM +0200, Andrea Bolognani wrote:
> The conversion has been performed by using pandoc as a first pass,
> and then tweaking the result manually until it looked satisfactory.
>
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
> build-aux/syntax-check.mk | 4 +-
> docs/hacking.html.in | 1555 -------------------------------------
> docs/hacking.rst | 1400 +++++++++++++++++++++++++++++++++
> 3 files changed, 1402 insertions(+), 1557 deletions(-)
> delete mode 100644 docs/hacking.html.in
> create mode 100644 docs/hacking.rst
Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>
> diff --git a/docs/hacking.rst b/docs/hacking.rst
> new file mode 100644
> index 0000000000..ac02e9ab73
> --- /dev/null
> +++ b/docs/hacking.rst
> @@ -0,0 +1,1400 @@
> +======================
> +Contributor guidelines
> +======================
> +
> +.. contents::
> +
> +General tips for contributing patches
> +=====================================
> +
> +#. Discuss any large changes on the mailing list first. Post
> + patches early and listen to feedback.
> +
> +#. Official upstream repository is kept in git
> + (``https://libvirt.org/git/libvirt.git``) and is browsable
> + along with other libvirt-related repositories (e.g.
> + libvirt-python) `online <https://libvirt.org/git/>`__.
> +
> +#. Patches to translations are maintained via the `zanata
> + project <https://fedora.zanata.org/>`__. If you want to fix a
> + translation in a .po file, join the appropriate language team.
> + The libvirt release process automatically pulls the latest
> + version of each translation file from zanata.
> +
> +#. The simplest way to send patches is to use the
> + `git-publish <https://github.com/stefanha/git-publish>`__
> + tool. All libvirt-related repositories contain a config file
> + that tells git-publish to use the correct mailing list and
> + subject prefix.
> +
> + Alternatively, you may send patches using ``git send-email``.
> +
> + Also, for code motion patches, you may find that
> + ``git diff --patience`` provides an easier-to-read
> + patch. However, the usual workflow of libvirt developer is:
> +
> + ::
> +
> + git checkout master
> + git pull
> + git checkout -t origin -b workbranch
> + Hack, committing any changes along the way
> +
> + More hints on compiling can be found `here <compiling.html>`__.
> + When you want to post your patches:
> +
> + ::
> +
> + git pull --rebase
> + (fix any conflicts)
> + git send-email --cover-letter --no-chain-reply-to --annotate \
> + --confirm=always --to=libvir-list at redhat.com master
> +
> + For a single patch you can omit ``--cover-letter``, but a
> + series of two or more patches needs a cover letter.
> +
> + Note that the ``git send-email`` subcommand may not be in the
> + main git package and using it may require installation of a
> + separate package, for example the "git-email" package in Fedora
> + and Debian. If this is your first time using
> + ``git send-email``, you might need to configure it to point it
> + to your SMTP server with something like:
> +
> + ::
> +
> + git config --global sendemail.smtpServer stmp.youremailprovider.net
> +
> + If you get tired of typing ``--to=libvir-list at redhat.com`` all
> + the time, you can configure that to be automatically handled as
> + well:
> +
> + ::
> +
> + git config sendemail.to libvir-list at redhat.com
> +
> + As a rule, patches should be sent to the mailing list only: all
> + developers are subscribed to libvir-list and read it regularly,
> + so **please don't CC individual developers** unless they've
> + explicitly asked you to.
> +
> + Avoid using mail clients for sending patches, as most of them
> + will mangle the messages in some way, making them unusable for
> + our purposes. Gmail and other Web-based mail clients are
> + particularly bad at this.
> +
> + If everything went well, your patch should show up on the
> + `libvir-list
> + archives <https://www.redhat.com/archives/libvir-list/>`__ in a
> + matter of minutes; if you still can't find it on there after an
> + hour or so, you should double-check your setup. **Note that, if
> + you are not already a subscriber, your very first post to the
> + mailing list will be subject to moderation**, and it's not
> + uncommon for that to take around a day.
> +
> + Please follow this as close as you can, especially the rebase
> + and ``git send-email`` part, as it makes life easier for other
> + developers to review your patch set.
> +
> + One should avoid sending patches as attachments, but rather
> + send them in email body along with commit message. If a
> + developer is sending another version of the patch (e.g. to
> + address review comments), they are advised to note differences
> + to previous versions after the ``---`` line in the patch so
> + that it helps reviewers but doesn't become part of git history.
> + Moreover, such patch needs to be prefixed correctly with
> + ``--subject-prefix=PATCHv2`` appended to
> + ``git send-email`` (substitute ``v2`` with the
> + correct version if needed though).
> +
> +#. In your commit message, make the summary line reasonably short
> + (60 characters is typical), followed by a blank line, followed
> + by any longer description of why your patch makes sense. If the
> + patch fixes a regression, and you know what commit introduced
> + the problem, mentioning that is useful. If the patch resolves a
> + bugzilla report, mentioning the URL of the bug number is
> + useful; but also summarize the issue rather than making all
> + readers follow the link. You can use 'git shortlog -30' to get
> + an idea of typical summary lines.
> +
> +#. Contributors to libvirt projects **must** assert that they are
> + in compliance with the `Developer Certificate of Origin
> + 1.1 <https://developercertificate.org/>`__. This is achieved by
> + adding a "Signed-off-by" line containing the contributor's name
> + and e-mail to every commit message. The presence of this line
> + attests that the contributor has read the above lined DCO and
> + agrees with its statements.
> +
> +#. Split large changes into a series of smaller patches,
> + self-contained if possible, with an explanation of each patch
> + and an explanation of how the sequence of patches fits
> + together. Moreover, please keep in mind that it's required to
> + be able to compile cleanly (**including**
> + ``make check`` and ``make syntax-check``) after each
> + patch. A feature does not have to work until the end of a
> + series, but intermediate patches must compile and not cause
> + test-suite failures (this is to preserve the usefulness of
> + ``git bisect``, among other things).
> +
> +#. Make sure your patches apply against libvirt GIT. Developers
> + only follow GIT and don't care much about released versions.
> +
> +#. Run the automated tests on your code before submitting any
> + changes. That is:
> +
> + ::
> +
> + make check
> + make syntax-check
> + make -C tests valgrind
> +
> + `Valgrind <http://valgrind.org/>`__ is a test that checks for
> + memory management issues, such as leaks or use of uninitialized
> + variables.
> +
> + Some tests are skipped by default in a development environment,
> + based on the time they take in comparison to the likelihood
> + that those tests will turn up problems during incremental
> + builds. These tests default to being run when building from a
> + tarball or with the configure option --enable-expensive-tests;
> + you can also force a one-time toggle of these tests by setting
> + VIR_TEST_EXPENSIVE to 0 or 1 at make time, as in:
> +
> + ::
> +
> + make check VIR_TEST_EXPENSIVE=1
> +
> + If you encounter any failing tests, the VIR_TEST_DEBUG
> + environment variable may provide extra information to debug the
> + failures. Larger values of VIR_TEST_DEBUG may provide larger
> + amounts of information:
> +
> + ::
> +
> + VIR_TEST_DEBUG=1 make check (or)
> + VIR_TEST_DEBUG=2 make check
> +
> + When debugging failures during development, it is possible to
> + focus in on just the failing subtests by using VIR_TEST_RANGE.
> + I.e. to run all tests from 3 to 20 with the exception of tests
> + 6 and 16, use:
> +
> + ::
> +
> + VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5,7-20,^16 ./run tests/qemuxml2argvtest
> +
> + Also, individual tests can be run from inside the ``tests/``
> + directory, like:
> +
> + ::
> +
> + ./qemuxml2xmltest
> +
> + If you are adding new test cases, or making changes that alter
> + existing test output, you can use the environment variable
> + VIR_TEST_REGENERATE_OUTPUT to quickly update the saved test
> + data. Of course you still need to review the changes VERY
> + CAREFULLY to ensure they are correct.
> +
> + ::
> +
> + VIR_TEST_REGENERATE_OUTPUT=1 ./qemuxml2argvtest
> +
> + There is also a ``./run`` script at the top level, to make it
> + easier to run programs that have not yet been installed, as
> + well as to wrap invocations of various tests under gdb or
> + Valgrind.
> +
> + When running our test suite it may happen that the test result
> + is nondeterministic because of the test suite relying on a
> + particular file in the system being accessible or having some
> + specific value. To catch this kind of errors, the test suite
> + has a module for that prints any path touched that fulfils
> + constraints described above into a file. To enable it just set
> + ``VIR_TEST_FILE_ACCESS`` environment variable. Then
> + ``VIR_TEST_FILE_ACCESS_OUTPUT`` environment variable can alter
> + location where the file is stored.
> +
> + ::
> +
> + VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest
> +
> +#. The Valgrind test should produce similar output to
> + ``make check``. If the output has traces within libvirt API's,
> + then investigation is required in order to determine the cause
> + of the issue. Output such as the following indicates some sort
> + of leak:
> +
> + ::
> +
> + ==5414== 4 bytes in 1 blocks are definitely lost in loss record 3 of 89
> + ==5414== at 0x4A0881C: malloc (vg_replace_malloc.c:270)
> + ==5414== by 0x34DE0AAB85: xmlStrndup (in /usr/lib64/libxml2.so.2.7.8)
> + ==5414== by 0x4CC97A6: virDomainVideoDefParseXML (domain_conf.c:7410)
> + ==5414== by 0x4CD581D: virDomainDefParseXML (domain_conf.c:10188)
> + ==5414== by 0x4CD8C73: virDomainDefParseNode (domain_conf.c:10640)
> + ==5414== by 0x4CD8DDB: virDomainDefParse (domain_conf.c:10590)
> + ==5414== by 0x41CB1D: testCompareXMLToArgvHelper (qemuxml2argvtest.c:100)
> + ==5414== by 0x41E20F: virtTestRun (testutils.c:161)
> + ==5414== by 0x41C7CB: mymain (qemuxml2argvtest.c:866)
> + ==5414== by 0x41E84A: virtTestMain (testutils.c:723)
> + ==5414== by 0x34D9021734: (below main) (in /usr/lib64/libc-2.15.so)
> +
> + In this example, the ``virDomainDefParseXML()`` had an error
> + path where the ``virDomainVideoDefPtr video`` pointer was not
> + properly disposed. By simply adding a
> + ``virDomainVideoDefFree(video);`` in the error path, the issue
> + was resolved.
> +
> + Another common mistake is calling a printing function, such as
> + ``VIR_DEBUG()`` without initializing a variable to be printed.
> + The following example involved a call which could return an
> + error, but not set variables passed by reference to the call.
> + The solution was to initialize the variables prior to the call.
> +
> + ::
> +
> + ==4749== Use of uninitialised value of size 8
> + ==4749== at 0x34D904650B: _itoa_word (in /usr/lib64/libc-2.15.so)
> + ==4749== by 0x34D9049118: vfprintf (in /usr/lib64/libc-2.15.so)
> + ==4749== by 0x34D9108F60: __vasprintf_chk (in /usr/lib64/libc-2.15.so)
> + ==4749== by 0x4CAEEF7: virVasprintf (stdio2.h:199)
> + ==4749== by 0x4C8A55E: virLogVMessage (virlog.c:814)
> + ==4749== by 0x4C8AA96: virLogMessage (virlog.c:751)
> + ==4749== by 0x4DA0056: virNetTLSContextCheckCertKeyUsage (virnettlscontext.c:225)
> + ==4749== by 0x4DA06DB: virNetTLSContextCheckCert (virnettlscontext.c:439)
> + ==4749== by 0x4DA1620: virNetTLSContextNew (virnettlscontext.c:562)
> + ==4749== by 0x4DA26FC: virNetTLSContextNewServer (virnettlscontext.c:927)
> + ==4749== by 0x409C39: testTLSContextInit (virnettlscontexttest.c:467)
> + ==4749== by 0x40AB8F: virtTestRun (testutils.c:161)
> +
> + Valgrind will also find some false positives or code paths
> + which cannot be resolved by making changes to the libvirt code.
> + For these paths, it is possible to add a filter to avoid the
> + errors. For example:
> +
> + ::
> +
> + ==4643== 7 bytes in 1 blocks are possibly lost in loss record 4 of 20
> + ==4643== at 0x4A0881C: malloc (vg_replace_malloc.c:270)
> + ==4643== by 0x34D90853F1: strdup (in /usr/lib64/libc-2.15.so)
> + ==4643== by 0x34EEC2C08A: ??? (in /usr/lib64/libnl.so.1.1)
> + ==4643== by 0x34EEC15B81: ??? (in /usr/lib64/libnl.so.1.1)
> + ==4643== by 0x34D8C0EE15: call_init.part.0 (in /usr/lib64/ld-2.15.so)
> + ==4643== by 0x34D8C0EECF: _dl_init (in /usr/lib64/ld-2.15.so)
> + ==4643== by 0x34D8C01569: ??? (in /usr/lib64/ld-2.15.so)
> +
> + In this instance, it is acceptable to modify the
> + ``tests/.valgrind.supp`` file in order to add a suppression
> + filter. The filter should be unique enough to not suppress real
> + leaks, but it should be generic enough to cover multiple code
> + paths. The format of the entry can be found in the
> + documentation found at the `Valgrind home
> + page <http://valgrind.org/>`__. The following trace was added
> + to ``tests/.valgrind.supp`` in order to suppress the warning:
> +
> + ::
> +
> + {
> + dlInitMemoryLeak1
> + Memcheck:Leak
> + fun:?alloc
> + ...
> + fun:call_init.part.0
> + fun:_dl_init
> + ...
> + obj:*/lib*/ld-2.*so*
> + }
> +
> +#. Update tests and/or documentation, particularly if you are
> + adding a new feature or changing the output of a program.
> +
> +#. Don't forget to update the `release notes <news.html>`__ by
> + changing ``docs/news.xml`` if your changes are significant. All
> + user-visible changes, such as adding new XML elements or fixing
> + all but the most obscure bugs, must be (briefly) described in a
> + release notes entry; changes that are only relevant to other
> + libvirt developers, such as code refactoring, don't belong in
> + the release notes. Note that ``docs/news.xml`` should be
> + updated in its own commit not to get in the way of backports.
> +
> +There is more on this subject, including lots of links to
> +background reading on the subject, on `Richard Jones' guide to
> +working with open source
> +projects <http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/>`__.
> +
> +Language Usage
> +==============
> +
> +The libvirt repository makes use of a large number of programming
> +languages. It is anticipated that in the future libvirt will adopt
> +use of other new languages. To reduce the overall burden on
> +developers, there is thus a general desire to phase out usage of
> +some of the existing languages.
> +
> +The preferred languages at this time are:
> +
> +- C - for the main libvirt codebase. Dialect supported by
> + GCC/CLang only.
> +- Python - for supporting build scripts / tools. Code must run
> + with both version 2.7 and 3.x at this time.
> +
> +Languages that should not be used for any new contributions:
> +
> +- Perl - build scripts must be written in Python instead.
> +- Shell - build scripts must be written in Python instead.
> +
> +Tooling
> +=======
> +
> +libvirt includes support for some useful development tools right
> +in its source repository, meaning users will be able to take
> +advantage of them without little or no configuration. Examples
> +include:
> +
> +- `color_coded <https://github.com/jeaye/color_coded>`__, a vim
> + plugin for libclang-powered semantic syntax highlighting;
> +- `YouCompleteMe <http://valloric.github.io/YouCompleteMe/>`__, a
> + vim plugin for libclang-powered semantic code completion.
> +
> +Naming conventions
> +==================
> +
> +When reading libvirt code, a number of different naming
> +conventions will be evident due to various changes in thinking
> +over the course of the project's lifetime. The conventions
> +documented below should be followed when creating any entirely new
> +files in libvirt. When working on existing files, while it is
> +desirable to apply these conventions, keeping a consistent style
> +with existing code in that particular file is generally more
> +important. The overall guiding principal is that every file, enum,
> +struct, function, macro and typedef name must have a 'vir' or
> +'VIR' prefix. All local scope variable names are exempt, and
> +global variables are exempt, unless exported in a header file.
> +
> +File names
> + File naming varies depending on the subdirectory. The preferred
> + style is to have a 'vir' prefix, followed by a name which
> + matches the name of the functions / objects inside the file.
> + For example, a file containing an object 'virHashtable' is
> + stored in files 'virhashtable.c' and 'virhashtable.h'.
> + Sometimes, methods which would otherwise be declared 'static'
> + need to be exported for use by a test suite. For this purpose a
> + second header file should be added with a suffix of 'priv',
> + e.g. 'virhashtablepriv.h'. Use of underscores in file names is
> + discouraged when using the 'vir' prefix style. The 'vir' prefix
> + naming applies to src/util, src/rpc and tests/ directories.
> + Most other directories do not follow this convention.
> +
> +Enum type & field names
> + All enums should have a 'vir' prefix in their typedef name, and
> + each following word should have its first letter in uppercase.
> + The enum name should match the typedef name with a leading
> + underscore. The enum member names should be in all uppercase,
> + and use an underscore to separate each word. The enum member
> + name prefix should match the enum typedef name.
> +
> + ::
> +
> + typedef enum _virSocketType virSocketType;
> + enum _virSocketType {
> + VIR_SOCKET_TYPE_IPV4,
> + VIR_SOCKET_TYPE_IPV6,
> + };
> +
> +Struct type names
> + All structs should have a 'vir' prefix in their typedef name,
> + and each following word should have its first letter in
> + uppercase. The struct name should be the same as the typedef
> + name with a leading underscore. A second typedef should be
> + given for a pointer to the struct with a 'Ptr' suffix.
> +
> + ::
> +
> + typedef struct _virHashTable virHashTable;
> + typedef virHashTable *virHashTablePtr;
> + struct _virHashTable {
> + ...
> + };
> +
> +Function names
> + All functions should have a 'vir' prefix in their name,
> + followed by one or more words with first letter of each word
> + capitalized. Underscores should not be used in function names.
> + If the function is operating on an object, then the function
> + name prefix should match the object typedef name, otherwise it
> + should match the filename. Following this comes the verb /
> + action name, and finally an optional subject name. For example,
> + given an object 'virHashTable', all functions should have a
> + name 'virHashTable$VERB' or 'virHashTable$VERB$SUBJECT", e.g.
> + 'virHashTableLookup' or 'virHashTableGetValue'.
> +
> +Macro names
> + All macros should have a "VIR" prefix in their name, followed
> + by one or more uppercase words separated by underscores. The
> + macro argument names should be in lowercase. Aside from having
> + a "VIR" prefix there are no common practices for the rest of
> + the macro name.
> +
> +Code indentation
> +================
> +
> +Libvirt's C source code generally adheres to some basic
> +code-formatting conventions. The existing code base is not totally
> +consistent on this front, but we do prefer that contributed code
> +be formatted similarly. In short, use spaces-not-TABs for
> +indentation, use 4 spaces for each indentation level, and other
> +than that, follow the K&R style.
> +
> +If you use Emacs, the project includes a file .dir-locals.el that
> +sets up the preferred indentation. If you use vim, append the
> +following to your ~/.vimrc file:
> +
> +::
> +
> + set nocompatible
> + filetype on
> + set autoindent
> + set smartindent
> + set cindent
> + set tabstop=8
> + set shiftwidth=4
> + set expandtab
> + set cinoptions=(0,:0,l1,t0,L3
> + filetype plugin indent on
> + au FileType make setlocal noexpandtab
> + au BufRead,BufNewFile *.am setlocal noexpandtab
> + match ErrorMsg /\s\+$\| \+\ze\t/
> +
> +Or if you don't want to mess your ~/.vimrc up, you can save the
> +above into a file called .lvimrc (not .vimrc) located at the root
> +of libvirt source, then install a vim script from
> +http://www.vim.org/scripts/script.php?script_id=1408, which will
> +load the .lvimrc only when you edit libvirt code.
> +
> +Code formatting (especially for new code)
> +=========================================
> +
> +With new code, we can be even more strict. Please apply the
> +following function (using GNU indent) to any new code. Note that
> +this also gives you an idea of the type of spacing we prefer
> +around operators and keywords:
> +
> +::
> +
> + indent-libvirt()
> + {
> + indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
> + -sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
> + --no-tabs "$@"
> + }
> +
> +Note that sometimes you'll have to post-process that output
> +further, by piping it through ``expand -i``, since some leading
> +TABs can get through. Usually they're in macro definitions or
> +strings, and should be converted anyhow.
> +
> +Libvirt requires a C99 compiler for various reasons. However, most
> +of the code base prefers to stick to C89 syntax unless there is a
> +compelling reason otherwise. For example, it is preferable to use
> +``/* */`` comments rather than ``//``. Also, when declaring local
> +variables, the prevailing style has been to declare them at the
> +beginning of a scope, rather than immediately before use.
> +
> +Bracket spacing
> +---------------
> +
> +The keywords ``if``, ``for``, ``while``, and ``switch`` must have
> +a single space following them before the opening bracket. E.g.
> +
> +::
> +
> + if(foo) // Bad
> + if (foo) // Good
> +
> +Function implementations must **not** have any whitespace between
> +the function name and the opening bracket. E.g.
> +
> +::
> +
> + int foo (int wizz) // Bad
> + int foo(int wizz) // Good
> +
> +Function calls must **not** have any whitespace between the
> +function name and the opening bracket. E.g.
> +
> +::
> +
> + bar = foo (wizz); // Bad
> + bar = foo(wizz); // Good
> +
> +Function typedefs must **not** have any whitespace between the
> +closing bracket of the function name and opening bracket of the
> +arg list. E.g.
> +
> +::
> +
> + typedef int (*foo) (int wizz); // Bad
> + typedef int (*foo)(int wizz); // Good
> +
> +There must not be any whitespace immediately following any opening
> +bracket, or immediately prior to any closing bracket. E.g.
> +
> +::
> +
> + int foo( int wizz ); // Bad
> + int foo(int wizz); // Good
> +
> +Commas
> +------
> +
> +Commas should always be followed by a space or end of line, and
> +never have leading space; this is enforced during 'make
> +syntax-check'.
> +
> +::
> +
> + call(a,b ,c);// Bad
> + call(a, b, c); // Good
> +
> +When declaring an enum or using a struct initializer that occupies
> +more than one line, use a trailing comma. That way, future edits
> +to extend the list only have to add a line, rather than modify an
> +existing line to add the intermediate comma. Any sentinel
> +enumerator value with a name ending in \_LAST is exempt, since you
> +would extend such an enum before the \_LAST element. Another
> +reason to favor trailing commas is that it requires less effort to
> +produce via code generators. Note that the syntax checker is
> +unable to enforce a style of trailing commas, so there are
> +counterexamples in existing code which do not use it; also, while
> +C99 allows trailing commas, remember that JSON and XDR do not.
> +
> +::
> +
> + enum {
> + VALUE_ONE,
> + VALUE_TWO // Bad
> + };
> + enum {
> + VALUE_THREE,
> + VALUE_FOUR, // Good
> + };
> +
> +Semicolons
> +----------
> +
> +Semicolons should never have a space beforehand. Inside the
> +condition of a ``for`` loop, there should always be a space or
> +line break after each semicolon, except for the special case of an
> +infinite loop (although more infinite loops use ``while``). While
> +not enforced, loop counters generally use post-increment.
> +
> +::
> +
> + for (i = 0 ;i < limit ; ++i) { // Bad
> + for (i = 0; i < limit; i++) { // Good
> + for (;;) { // ok
> + while (1) { // Better
> +
> +Empty loop bodies are better represented with curly braces and a
> +comment, although use of a semicolon is not currently rejected.
> +
> +::
> +
> + while ((rc = waitpid(pid, &st, 0) == -1) &&
> + errno == EINTR); // ok
> + while ((rc = waitpid(pid, &st, 0) == -1) &&
> + errno == EINTR) { // Better
> + /* nothing */
> + }
> +
> +Curly braces
> +------------
> +
> +Omit the curly braces around an ``if``, ``while``, ``for`` etc.
> +body only when both that body and the condition itself occupy a
> +single line. In every other case we require the braces. This
> +ensures that it is trivially easy to identify a
> +single-\ *statement* loop: each has only one *line* in its body.
> +
> +::
> +
> + while (expr) // single line body; {} is forbidden
> + single_line_stmt();
> +
> +::
> +
> + while (expr(arg1,
> + arg2)) // indentation makes it obvious it is single line,
> + single_line_stmt(); // {} is optional (not enforced either way)
> +
> +::
> +
> + while (expr1 &&
> + expr2) { // multi-line, at same indentation, {} required
> + single_line_stmt();
> + }
> +
> +However, the moment your loop/if/else body extends on to a second
> +line, for whatever reason (even if it's just an added comment),
> +then you should add braces. Otherwise, it would be too easy to
> +insert a statement just before that comment (without adding
> +braces), thinking it is already a multi-statement loop:
> +
> +::
> +
> + while (true) // BAD! multi-line body with no braces
> + /* comment... */
> + single_line_stmt();
> +
> +Do this instead:
> +
> +::
> +
> + while (true) { // Always put braces around a multi-line body.
> + /* comment... */
> + single_line_stmt();
> + }
> +
> +There is one exception: when the second body line is not at the
> +same indentation level as the first body line:
> +
> +::
> +
> + if (expr)
> + die("a diagnostic that would make this line"
> + " extend past the 80-column limit"));
> +
> +It is safe to omit the braces in the code above, since the
> +further-indented second body line makes it obvious that this is
> +still a single-statement body.
> +
> +To reiterate, don't do this:
> +
> +::
> +
> + if (expr) // BAD: no braces around...
> + while (expr_2) { // ... a multi-line body
> + ...
> + }
> +
> +Do this, instead:
> +
> +::
> +
> + if (expr) {
> + while (expr_2) {
> + ...
> + }
> + }
> +
> +However, there is one exception in the other direction, when even
> +a one-line block should have braces. That occurs when that
> +one-line, brace-less block is an ``if`` or ``else`` block, and the
> +counterpart block **does** use braces. In that case, put braces
> +around both blocks. Also, if the ``else`` block is much shorter
> +than the ``if`` block, consider negating the ``if``-condition and
> +swapping the bodies, putting the short block first and making the
> +longer, multi-line block be the ``else`` block.
> +
> +::
> +
> + if (expr) {
> + ...
> + ...
> + }
> + else
> + x = y; // BAD: braceless "else" with braced "then",
> + // and short block last
> +
> + if (expr)
> + x = y; // BAD: braceless "if" with braced "else"
> + else {
> + ...
> + ...
> + }
> +
> +Keeping braces consistent and putting the short block first is
> +preferred, especially when the multi-line body is more than a few
> +lines long, because it is easier to read and grasp the semantics
> +of an if-then-else block when the simpler block occurs first,
> +rather than after the more involved block:
> +
> +::
> +
> + if (!expr) {
> + x = y; // putting the smaller block first is more readable
> + } else {
> + ...
> + ...
> + }
> +
> +But if negating a complex condition is too ugly, then at least add
> +braces:
> +
> +::
> +
> + if (complex expr not worth negating) {
> + ...
> + ...
> + } else {
> + x = y;
> + }
> +
> +Use hanging braces for compound statements: the opening brace of a
> +compound statement should be on the same line as the condition
> +being tested. Only top-level function bodies, nested scopes, and
> +compound structure declarations should ever have { on a line by
> +itself.
> +
> +::
> +
> + void
> + foo(int a, int b)
> + { // correct - function body
> + int 2d[][] = {
> + { // correct - complex initialization
> + 1, 2,
> + },
> + };
> + if (a)
> + { // BAD: compound brace on its own line
> + do_stuff();
> + }
> + { // correct - nested scope
> + int tmp;
> + if (a < b) { // correct - hanging brace
> + tmp = b;
> + b = a;
> + a = tmp;
> + }
> + }
> + }
> +
> +Conditional expressions
> +-----------------------
> +
> +For readability reasons new code should avoid shortening
> +comparisons to 0 for numeric types. Boolean and pointer
> +comparisions may be shortened. All long forms are okay:
> +
> +::
> +
> + virFooPtr foos = NULL;
> + size nfoos = 0;
> + bool hasFoos = false;
> +
> + GOOD:
> + if (!foos)
> + if (!hasFoos)
> + if (nfoos == 0)
> + if (foos == NULL)
> + if (hasFoos == true)
> +
> + BAD:
> + if (!nfoos)
> + if (nfoos)
> +
> +New code should avoid the ternary operator as much as possible.
> +Specifically it must never span more than one line or nest:
> +
> +::
> +
> + BAD:
> + char *foo = baz ?
> + virDoSomethingReallyComplex(driver, vm, something, baz->foo) :
> + NULL;
> +
> + char *foo = bar ? bar->baz ? bar->baz->foo : "nobaz" : "nobar";
> +
> +Preprocessor
> +------------
> +
> +Macros defined with an ALL_CAPS name should generally be assumed
> +to be unsafe with regards to arguments with side-effects (that is,
> +MAX(a++, b--) might increment a or decrement b too many or too few
> +times). Exceptions to this rule are explicitly documented for
> +macros in viralloc.h and virstring.h.
> +
> +For variadic macros, stick with C99 syntax:
> +
> +::
> +
> + #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
> +
> +Use parenthesis when checking if a macro is defined, and use
> +indentation to track nesting:
> +
> +::
> +
> + #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
> + # define fallocate(a, ignored, b, c) posix_fallocate(a, b, c)
> + #endif
> +
> +C types
> +-------
> +
> +Use the right type.
> +
> +Scalars
> +^^^^^^^
Our style guide (docs/styleguide.rst) calls for "~~~~~~~" for level 4
> +Pointers
> +^^^^^^^^
With the two headings changed
Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>
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 :|
More information about the libvir-list
mailing list