Re: [libvirt PATCH 01/10] docs: Convert hacking.html to reStructuredText

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux