[Spice-devel] [PATCH] Proposed changes to the style guide

[Frediano Ziglio]

> 
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  docs/spice_style.txt | 113
>  ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 81 insertions(+), 32 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index eb0e30ef..8e2e7363 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -1,7 +1,7 @@
>  Spice project coding style and coding conventions
>  =================================================
>  
> -Copyright (C) 2009-2016 Red Hat, Inc.
> +Copyright (C) 2009-2018 Red Hat, Inc.
>  Licensed under a Creative Commons Attribution-Share Alike 3.0
>  United States License (see
>  http://creativecommons.org/licenses/by-sa/3.0/us/legalcode).
>  
> @@ -14,7 +14,16 @@ Names
>  
>  Use lower case and separate words using dashes (e.g., file-name.c,
>  header.h).
>  
> -Use standard file extension for C source and header files.
> +The file extensions used in the SPICE project are:
> +- .c for C source
> +- .cpp for C++ sources
> +- .h for headers that can be included from C code
> +- .hpp for headers that are strictly reserved to C++
> +- .m for Objective-C source files (currently not properly enforced)
> +
> +Note that .h headers may contain C++ code as long as the header can
> +sucessfully be included from a C source file.
> +

typo "sucessfully".
Why .m ? Can't wait ? :-) 

>  
>  Line width
>  ~~~~~~~~~~
> @@ -73,7 +82,11 @@ Comments that are prefixed with `FIXME` describe a bug
> that need to be fixed. Ge
>  ASSERT
>  ------
>  
> -Use it freely. ASSERT helps testing function arguments and function results
> validity.  ASSERT helps detecting bugs and improve code readability and
> stability.
> +Use assertions liberally. They helps testing function arguments and function
> results validity. Assertions helps detecting bugs and improve code
> readability and stability.
> +
> +Several form of assertion exist, notably:
> +- spice_assert which should be preferred for any assertion related to SPICE
> itself
> +- glib asserts (many forms) which should be preferred for any assertion
> related to the use of glib
>  

Actually I think the original ASSERT here were supposed to be more like
Windows. Note that on SPICE we never use assert as C, we always compile
them in.

>  sizeof
>  ------
> @@ -93,12 +106,14 @@ Using goto is allowed in C code for error handling. In
> any other case it will be
>  Defining Constant values
>  ------------------------
>  
> -Use defines for constant values for improving readability and ease of
> changes. Alternatively, use global `const` variables.
> +Use defines for constant values for improving readability and ease of
> changes.
> +Alternatively, use global `const` variables for individual values.
> +If multiple related constants are to be defined, consider the use of
> enumerations with initializers.
>  
>  Short functions
>  ---------------
>  
> -Try to split code to short functions, each having simple functionality, in
> order to improve code readability and re-usability. Prefix with inline short
> functions or functions that were splitted for readability reason.
> +Try to split code to short functions, each having simple functionality, in
> order to improve code readability and re-usability. Prefix with `inline`
> functions that were splitted for readability reason or that are very short.
>  
>  Return on if
>  ------------

Too much changes in a single patch, please split!

> @@ -118,7 +133,8 @@ void function(int *n)
>      ...
>  }
>  ----
> -on
> +over
> +
>  [source,c]
>  ----
>  void function(int *n)
> @@ -238,15 +254,7 @@ if (condition) {
>  +
>  In case of long condition statement, prefer having additional temporary
>  variable over multiple line condition statement.
>  +
> -In case of new line in condition statement.
> -+
> -[source,c]
> -----
> -if (long_name && very_long_name && very_long ||
> -                                               var_name) {
> -----
> -+
> -or indent under the round bracket using spaces
> +Indent long conditionals under the opening parenthesis using spaces
>  +
>  [source,c]
>  ----

Why this removal?

> @@ -285,6 +293,8 @@ default:
>  }
>  ----
>  
> +Use /* Fall through */ comments when there is no break (compilers will emit
> a warning otherwise)
> +
>  Types indentation
>  ~~~~~~~~~~~~~~~~~
>  
> @@ -330,7 +340,7 @@ Multi line macro indentation
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
>  [source,c]
> -#define MACRO_NAME(a, b, c) {        \
> +#define MACRO_NAME(a, b, c) {   \
>      int ab = a + c;             \
>      int ac = a + b;             \
>      int bc = b + c;             \
> @@ -347,35 +357,74 @@ char *array[] = {
>      "item_3",
>  };
>  
> +Headers
> +-------
> +
> +Headers should be protected against multiple inclusing using a macro that

typo, "inclusing"

> matches the header file name in uppercase, with all characters that are

should be "upper case"

> invalid in C replaced with an underscore '_':
> +
> +[source,h]
> +---
> +#ifndef MY_MODULE_H
> +#define MY_MODULE_H
> +
> +...
> +
> +#endif /* MY_MODULE_H */
> +---
> +
> +
>  Header inclusion
>  ----------------
>  
> -Headers should be included in this order
> +Headers should be included in this order:
> +- config.h, which should only be included from C source files
> +- [module].h, where [module].c is the corresponding implementation file
> +- [module]-xyz.h, which are support headers for [module]
> +- Other application headers, using #include "file.h"
> +- System headers, using #include <file.h>
> +- If necessary, C++ system headers, using #include <file>
> +
> +This order is designed to maximize chances of catching missing headers in
> headers (i.e. headers that are not self-contained).
> +
> +In summary, Headers should be included in this order
>  
>  [source,c]
>  ----
> -#include <system_headers.h>
> -#include <no_spice_no_system_libraries.h>
> +#include "config.h"
> +#include "source.h"
> +#include "source-support.h"
> +#include "some-other-source.h"
> +
>  #include <spice_protocol.h>
>  #include <spice_common.h>
> -
> -#include "spice_server.h"
> +#include <no_spice_no_system_libraries.h>
> +#include <system_headers.h>
> +#include <vector>
> +#include <cstdio>
>  ----
>  
> -(note the empty line between no spice-server and spice-server headers)
> +(note the empty line between application headers included with "" and system
> headers included with <>
>  
> -Also in source (no header) files you must include `config.h` at the
> beginning so should start (beside comments and copyright) with
> +Headers should include only the headers required to process the header
> itself, and otherwise include as little as possible.
>  
> -[source,c]
> +[source,h]
>  ----
> -#ifdef HAVE_CONFIG_H
> -#include <config.h>
> -#endif
> +#ifndef SOURCE_H
> +#define SOURCE_H
> +#include "application-header-required-for-header.h"
>  
> -#include <system_headers.h>
> -#include <no_spice_no_system_libraries.h>
> -#include <spice_protocol.h>
> -#include <spice_common.h>
> +#include <system-header-required-for-header.h>
> +
> +...
>  
> -#include "spice_server.h"
> +#endif /* SOURCE_H */

I think this header changes applied everywhere should have more consents.
Another reason to split this too long patch!

>  ----
> +
> +
> +Compilation
> +-----------
> +
> +The source code should compile without warnings on all variants of GCC and
> clang available.

No, please!

> +A patch may be rejected if it introduces new warnings.
> +Warnings that appear over time due to improvements in compilers should be
> fixed in dedicated patches. A patch should not mix warning fixes and other
> changes.
> +Any patch may adjust whitespace (e.g. eliminate trailing whitespace).
> Whitespace adjustments do not require specific patches.

I would agree only if the changes touch these lines.
Otherwise I disagree.

Frediano