[Spice-devel] [PATCH] Proposed changes to the style guide
Christophe de Dinechin
christophe.de.dinechin at gmail.com
Wed Feb 7 14:54:07 UTC 2018
> On 7 Feb 2018, at 14:58, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Wed, 2018-02-07 at 14:44 +0100, Christophe de Dinechin wrote:
>>> On 7 Feb 2018, at 12:12, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>>>
>>> On Wed, 2018-02-07 at 11:35 +0100, Christophe de Dinechin wrote:
>>>>> On 7 Feb 2018, at 11:01, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>>>
>>>>>>
>>>>>> 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 ? :-)
>>>>
>>>> While I was looking at it, I thought I’d mention it.
>>>> AFAIK, there is only one file that should be renamed, vncdisplaykeymap_xorgxquartz2xtkbd.c.
>>>> That file really needs Obj-C
>>>>
>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>> Does not matter with respect to what I wrote, does it? Or would you like to rephrase?
>>>>
>>>>>
>>>>>> 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!
>>>>
>>>> Seriously?
>>>>
>>>> If you are serious, I can submit a series, though I think this makes things harder to review.
>>>
>>> As a side note, I think a single patch is fine here :)
>>>
>>>>>
>>>>>> @@ -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?
>>>>
>>>> I could not understand what it meant. I thought it was an error. Is it not?
>>>> Can you point me to one example in the source code where it’s indented like this?
>>>> Also, that breaks all auto-indent tools I know of. So I would strongly suggest we remove the rule if this ever was one.
>>>>
>>>>>
>>>>>> @@ -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"
>>>>
>>>> I had seen it (and fixed locally)
>>>>
>>>>>
>>>>>> matches the header file name in uppercase, with all characters that are
>>>>>
>>>>> should be "upper case”
>>>>
>>>> Both spellings are valid, see https://www.thefreedictionary.com/uppercase
>>>>
>>>>>
>>>>>> 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!
>>>>
>>>> What do you mean “more consents” (I don’t think this is correct english)? Do you mean we need to discuss it more?
>>>
>>> We always used the header order in Christophe's patch in my previous
>>> job. I was surprised to see the reverse order in our style guide, but
>>> thought there are more important reasons than ensuring correctness,
>>> like some #defines changing behaviour...
>>>
>>>> By the way, I am not suggesting we start reworking the code to match that, just that we agree on whether this is the correct way or not. As for refactoring, it’s actually easy because clang-format does it for you, and Emacs has a clang-format that applies to a selection, so the job is relatively straightforward.
>>>>
>>>> Also note that clang-format does imply a few style changes as well, for things that are not clearly specified in the style guide. Let me just give an example:
>>>>
>>>> GType
>>>> spice_compat_version_t_spice_compat_version_t_get_type (void)
>>>> {
>>>> static GType type = 0;
>>>> static volatile gsize type_volatile = 0;
>>>>
>>>> if (g_once_init_enter(&type_volatile)) {
>>>> type = g_enum_register_static ("spice_compat_version_t", _spice_compat_version_t_spice_compat_version_t_values);
>>>> g_once_init_leave(&type_volatile, type);
>>>> }
>>>>
>>>> return type;
>>>> }
>>>>
>>>> with the LLVM style adjusted for explicit preferences in the document, turns into
>>>>
>>>> GType spice_compat_version_t_spice_compat_version_t_get_type(void)
>>>> {
>>>> static GType type = 0;
>>>> static volatile gsize type_volatile = 0;
>>>>
>>>> if (g_once_init_enter(&type_volatile))
>>>> {
>>>> type = g_enum_register_static("spice_compat_version_t",
>>>> _spice_compat_version_t_spice_compat_version_t_values);
>>>> g_once_init_leave(&type_volatile, type);
>>>> }
>>>>
>>>> return type;
>>>> }
>>>
>>> How does this relate to the headers?
>>
>> Sorry, a short-circuit in my brain that was all but obvious…
>>
>> The relation is clang-format.
>>
>> clang-format can do the header reordering. I added a .clang-format in the latest iteration of the patch series.
>>
>> It also enforces a number of other rules in a way that I personally find quite nice overall if applied locally using some IDE. I would not recommend applying it globally to entire files.
>
> That might be quite nice... In that case I'd like to nitpick the
> bracket on a new line after the if statement :) Don't think we want
> that?
Talk about yourself ;-)
But you are right, this is not what the team decided. I need to find what replaces “Altman”
Christophe
>
>>
>>>
>>>> So it puts the function definition { at end of line. Not sure what the team prefers here. It also rewrites the function call to split arguments, which I think is nicer that way
>>>>
>>>> FYI, this is with the following in .clang-format:
>>>>
>>>> Language: Cpp
>>>> # BasedOnStyle: LLVM
>>>>
>>>> # IncludeBlocks: Regroup
>>>> SortIncludes: true
>>>>
>>>> IncludeCategories:
>>>> - Regex: 'config.h'
>>>> Priority: -1
>>>> - Regex: '^"spice.*"'
>>>> Priority: 1
>>>> - Regex: 'glib'
>>>> Priority: 4
>>>> - Regex: '^<.*>'
>>>> Priority: 3
>>>> - Regex: '^".*"'
>>>> Priority: 2
>>>>
>>>> ColumnLimit: 100
>>>> IndentCaseLabels: false
>>>> IndentWidth: 4
>>>> BreakBeforeBraces: Allman
>>>>
>>>>
>>>> I disabled InlcudeBlocks: Regroup which is not available unless you run a very recent clang-fromat.
>>>>
>>>>>
>>>>>> ----
>>>>>> +
>>>>>> +
>>>>>> +Compilation
>>>>>> +-----------
>>>>>> +
>>>>>> +The source code should compile without warnings on all variants of GCC and
>>>>>> clang available.
>>>>>
>>>>> No, please!
>>>>
>>>> That’s a should. Why not? We enforce that in our makefiles.
>>>>
>>>>>
>>>>>> +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.
>>>>
>>>> Having some of my patches rejected because of spurious whitespace changes is extremely annoying. I can easily configure Emacs to remove all trailing spaces when saving. I cannot configure it to remove spaces only on lines that I changed. I’d much rather slowly fix spurious whitespaces over time automatically than be forced to do some extra work just because you don’t want to see whitespace fixes in my patches.
>>>>
>>>> In short, please don’t make my life miserable for something that is no extra work for you.
>>>
>>> FWIW in vim I have a piece of config that highlights the trailing
>>> whitespaces. That way I can remove them manually where I want and also
>>> makes me not create them unintentionally.
>>
>> Emacs has a mode like that (whitespace-mode) but I find it visually distracting. For my own projects, I wanted to make sure I was not committing trailing whitespace, so I added this to my .emacs:
>
> Out of curiosity, why do you find it distracting? I never ever see the
> highlight, it doesn't show for the current line when I'm in edit mode
> in vim (wink, nudge :D). It only shows when I move to another line or
> quit edit mode...
>
>> (add-hook 'before-save-hook 'delete-trailing-whitespace)
>>
>> I find it quite inconvenient to disable this setting while working on SPICE projects just for the sake of not accidentally removing whitespaces we said we did not want anyway ;-)
>
> In that case, why don't we just remove all trailing whitespaces in one
> commit and be done with it? Easy enough.
>
> Lukas
>
>>
>>>
>>> Unless you have messy source with lots of them, this is fine (and if
>>> you do, a quick sed solves the problem).
>>>
>>> I can share it if you want (it's google-able, but vim wiki has a wall
>>> of text on it and you need to fiddle...)
>>>
>>> Lukas
>>>
>>>>>
>>>>> Frediano
>>>>
>>>> _______________________________________________
>>>> Spice-devel mailing list
>>>> Spice-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list