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

[Christophe de Dinechin]

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.
+
 
 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
 
 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
 ------------
@@ -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]
 ----
@@ -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 matches the header file name in uppercase, with all characters that are 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 */
 ----
+
+
+Compilation
+-----------
+
+The source code should compile without warnings on all variants of GCC and clang available.
+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.
-- 
2.13.5 (Apple Git-94)