On Wed, 2018-02-07 at 14:44 +0100, Christophe de Dinechin wrote:
> > 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.
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?
>
> >
> > > 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
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
_______________________________________________
Spice-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/spice-devel
