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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> > 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@xxxxxxxxxx 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@xxxxxxxxxx`` all > + the time, you can configure that to be automatically handled as > + well: > + > + :: > + > + git config sendemail.to libvir-list@xxxxxxxxxx > + > + 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@xxxxxxxxxx> 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 :|
