> On 7 Feb 2018, at 14:58, Lukáš Hrázký <lhra...@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ý <lhra...@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 <fzig...@redhat.com> wrote:
>>>>>
>>>>>>
>>>>>> From: Christophe de Dinechin <dinec...@redhat.com>
>>>>>>
>>>>>> Signed-off-by: Christophe de Dinechin <dinec...@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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
