> On 7 Feb 2018, at 12:12, Lukáš Hrázký <[email protected]> wrote:
>
> On Wed, 2018-02-07 at 11:35 +0100, Christophe de Dinechin wrote:
>>> On 7 Feb 2018, at 11:01, Frediano Ziglio <[email protected]> wrote:
>>>
>>>>
>>>> From: Christophe de Dinechin <[email protected]>
>>>>
>>>> Signed-off-by: Christophe de Dinechin <[email protected]>
>>>> ---
>>>> 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.
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:
(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 ;-)
Christophe
>
> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/spice-devel
