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?
> 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.
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
