[Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
From: Christophe de Dinechin As written, the headers style guide looks quite wrong. In particular, it places headers in an order that makes it hard to detect hidden dependencies in SPICE headers. These rules can be enforced by the .clang-format proposed in earlier patch, locally if you use the Emacs clang-format.el integration supplied with LLVM. Signed-off-by: Christophe de Dinechin --- docs/spice_style.txt | 44 +--- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/docs/spice_style.txt b/docs/spice_style.txt index 108a57a5..ff505b2a 100644 --- a/docs/spice_style.txt +++ b/docs/spice_style.txt @@ -407,34 +407,48 @@ Historically, some headers added underscores liberally, e.g. MY_MODULE_H_. This 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 +- If necessary, C++ system headers, using #include + +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 -#include +#include "config.h" +#include "source.h" +#include "source-support.h" +#include "some-other-source.h" + #include #include - -#include "spice_server.h" +#include +#include +#include +#include -(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] -#ifdef HAVE_CONFIG_H -#include -#endif +#ifndef SOURCE_H +#define SOURCE_H +#include "application-header-required-for-header.h" -#include -#include -#include -#include +#include + +... -#include "spice_server.h" +#endif /* SOURCE_H */ -- 2.13.5 (Apple Git-94) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
This one sounds more like an RFC to me, as from a quick look in server/, this is not the style currently in use. Christophe On Thu, Feb 08, 2018 at 12:25:31PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > As written, the headers style guide looks quite wrong. In particular, > it places headers in an order that makes it hard to detect hidden > dependencies in SPICE headers. > > These rules can be enforced by the .clang-format proposed in earlier patch, > locally if you use the Emacs clang-format.el integration supplied with LLVM. > > Signed-off-by: Christophe de Dinechin > --- > docs/spice_style.txt | 44 +--- > 1 file changed, 29 insertions(+), 15 deletions(-) > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt > index 108a57a5..ff505b2a 100644 > --- a/docs/spice_style.txt > +++ b/docs/spice_style.txt > @@ -407,34 +407,48 @@ Historically, some headers added underscores liberally, > e.g. MY_MODULE_H_. This > 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 > +- If necessary, C++ system headers, using #include > + > +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 > -#include > +#include "config.h" > +#include "source.h" > +#include "source-support.h" > +#include "some-other-source.h" > + > #include > #include > - > -#include "spice_server.h" > +#include > +#include > +#include > +#include > > > -(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] > > -#ifdef HAVE_CONFIG_H > -#include > -#endif > +#ifndef SOURCE_H > +#define SOURCE_H > +#include "application-header-required-for-header.h" > > -#include > -#include > -#include > -#include > +#include > + > +... > > -#include "spice_server.h" > +#endif /* SOURCE_H */ > > > > -- > 2.13.5 (Apple Git-94) > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
> On 14 Feb 2018, at 14:35, Christophe Fergeau wrote:
>
> This one sounds more like an RFC to me
Well, this is really a bug fix in the documentation more than a RFC.
> , as from a quick look in server/,
> this is not the style currently in use.
As I pointed out in earlier discussions, this section of the style guide, as
written currently, is mostly backwards compared to industry best practices.
Most projects today put project headers first for a reason: it catches the
frequent case where a header change makes it not self-contained (therefore
making it possible to break third-party code using that header).
Examples of explicit recommendations to that effect from various heavyweights:
1. LLVM: https://llvm.org/docs/CodingStandards.html:
>
> We prefer these #includes to be listed in this order:
>
> • Main Module Header
> • Local/Private Headers
> • LLVM project/subproject headers (clang/..., lldb/..., llvm/..., etc)
> • System #includes
>
2. Google:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test
> the stuff in dir2/foo2.h, order your includes as follows:
>
> • dir2/foo2.h.
> • C system files.
> • C++ system files.
> • Other libraries' .h files.
> • Your project's .h files.
> With the preferred ordering, if dir2/foo2.h omits any necessary includes, the
> build of dir/foo.cc or dir/foo_test.cc will break. Thus, this rule ensures
> that build breaks show up first for the people working on these files, not
> for innocent people in other packages.
>
3. Mozilla:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
> • Includes are split into three blocks, and sorted alphabetically in
> each block:
> • The main header: #include "Foo.h" in Foo.cpp
> • Standard library includes: #include
> • Mozilla includes: #include "mozilla/dom/Element.h”
I prefer the LLVM order personally, because it tends to catch errors slightly
faster and with a little less noise in the middle (system headers being assumed
“good”).
For example, let’s say I forget a closing } in a namespace in
frame-capture.hpp. With the proposed order, the error message is:
—— 8<
spice-streaming-agent.cpp:562:2: error: expected '}'
}
^
../include/spice-streaming-agent/frame-capture.hpp:13:17: note: to match this
'{'
namespace spice {
—— 8<
With the current order or the Google order, the error message is:
—— 8<
In file included from spice-streaming-agent.cpp:38:
In file included from ./concrete-agent.hpp:10:
In file included from
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/set:389:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__tree:535:35:
error: unknown class name 'false_type'; did you mean '::std::false_type'?
struct __is_tree_value_type_imp : false_type {};
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:526:38:
note: '::std::false_type' declared here
typedef _LIBCPP_BOOL_CONSTANT(false) false_type;
^
In file included from spice-streaming-agent.cpp:38:
In file included from ./concrete-agent.hpp:10:
In file included from
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/set:389:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__tree:538:63:
error: unknown class name 'true_type'; did you mean '::std::true_type'?
struct __is_tree_value_type_imp<__value_type<_Key, _Value>> : true_type {};
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:525:38:
note: '::std::true_type' declared here
typedef _LIBCPP_BOOL_CONSTANT(true) true_type;
^
In file included from spice-streaming-agent.cpp:38:
In file included from ./concrete-agent.hpp:10:
In file included from
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/set:389:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__tree:541:31:
error: unknown class name 'false_type'; did you mean '::std::false_type'?
struct __is_tree_value_type : false_type {};
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:526:38:
note: '::std::false_type' declared here
typedef _LIBCPP_BOOL_CONSTANT(false) false_type;
^
In file included from spice-st
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 14:35, Christophe Fergeau wrote: > > > > This one sounds more like an RFC to me > > Well, this is really a bug fix in the documentation more than a RFC. > > > , as from a quick look in server/, > > this is not the style currently in use. > > As I pointed out in earlier discussions, this section of the style guide, as > written currently, is mostly backwards compared to industry best practices. > > Most projects today put project headers first for a reason: it catches the > frequent case where a header change makes it not self-contained (therefore > making it possible to break third-party code using that header). > > Examples of explicit recommendations to that effect from various heavyweights: I haven't said I am against this style What I mean is that spice-server is not following this style, as far as I can tell spice-gtk is not either, so adding this now to the coding styles is just odd. It should reflect at least a little bit what is currently being done ;) If we want to change and decide that's important enough, then I would go with a separate patch series which would change at least part of the codebase to follow that style, and then add this to the coding style doc. Though I don't think it's really useful to spend a lot of time on it... Alternatively, this section of the coding style should acknowledge that this is not followed at all by the current code base, but that new code should do that, and explain that when making changes to a file, patches reordering the headers in that file are welcome (if that's how we want to deal with the change). Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
On Wed, 2018-02-14 at 23:24 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 14:35, Christophe Fergeau wrote:
> >
> > This one sounds more like an RFC to me
>
> Well, this is really a bug fix in the documentation more than a RFC.
>
> > , as from a quick look in server/,
> > this is not the style currently in use.
>
> As I pointed out in earlier discussions, this section of the style guide, as
> written currently, is mostly backwards compared to industry best practices.
>
> Most projects today put project headers first for a reason: it catches the
> frequent case where a header change makes it not self-contained (therefore
> making it possible to break third-party code using that header).
>
> Examples of explicit recommendations to that effect from various heavyweights:
>
> 1. LLVM: https://llvm.org/docs/CodingStandards.html:
> >
> > We prefer these #includes to be listed in this order:
> >
> > • Main Module Header
> > • Local/Private Headers
> > • LLVM project/subproject headers (clang/..., lldb/..., llvm/..., etc)
> > • System #includes
> >
>
>
> 2. Google:
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
>
> > In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or
> > test the stuff in dir2/foo2.h, order your includes as follows:
> >
> > • dir2/foo2.h.
> > • C system files.
> > • C++ system files.
> > • Other libraries' .h files.
> > • Your project's .h files.
> > With the preferred ordering, if dir2/foo2.h omits any necessary includes,
> > the build of dir/foo.cc or dir/foo_test.cc will break. Thus, this rule
> > ensures that build breaks show up first for the people working on these
> > files, not for innocent people in other packages.
> >
>
>
> 3. Mozilla:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
> > • Includes are split into three blocks, and sorted alphabetically in
> > each block:
> > • The main header: #include "Foo.h" in Foo.cpp
> > • Standard library includes: #include
> > • Mozilla includes: #include "mozilla/dom/Element.h”
>
>
>
> I prefer the LLVM order personally, because it tends to catch errors slightly
> faster and with a little less noise in the middle (system headers being
> assumed “good”).
>
> For example, let’s say I forget a closing } in a namespace in
> frame-capture.hpp. With the proposed order, the error message is:
>
> —— 8<
> spice-streaming-agent.cpp:562:2: error: expected '}'
> }
> ^
> ../include/spice-streaming-agent/frame-capture.hpp:13:17: note: to match this
> '{'
> namespace spice {
> —— 8<
>
>
> With the current order or the Google order, the error message is:
>
> —— 8<
>
> In file included from spice-streaming-agent.cpp:38:
> In file included from ./concrete-agent.hpp:10:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/set:389:
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__tree:535:35:
> error: unknown class name 'false_type'; did you mean '::std::false_type'?
> struct __is_tree_value_type_imp : false_type {};
> ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:526:38:
> note: '::std::false_type' declared here
> typedef _LIBCPP_BOOL_CONSTANT(false) false_type;
> ^
> In file included from spice-streaming-agent.cpp:38:
> In file included from ./concrete-agent.hpp:10:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/set:389:
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__tree:538:63:
> error: unknown class name 'true_type'; did you mean '::std::true_type'?
> struct __is_tree_value_type_imp<__value_type<_Key, _Value>> : true_type {};
> ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:525:38:
> note: '::std::true_type' declared here
> typedef _LIBCPP_BOOL_CONSTANT(true) true_type;
> ^
> In file included from spice-streaming-agent.cpp:38:
> In file included from ./concrete-agent.hpp:10:
> In file included from
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/set:389:
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__tree:541:31:
> error: unknown class name 'false_type'; did you mean '::std::false_type'?
> struct __is_tree_value_type : false_type {};
> ^
> /Applications/Xcod
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
> On 15 Feb 2018, at 10:19, Christophe Fergeau wrote: > > On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote: >> >> >>> On 14 Feb 2018, at 14:35, Christophe Fergeau wrote: >>> >>> This one sounds more like an RFC to me >> >> Well, this is really a bug fix in the documentation more than a RFC. >> >>> , as from a quick look in server/, >>> this is not the style currently in use. >> >> As I pointed out in earlier discussions, this section of the style guide, as >> written currently, is mostly backwards compared to industry best practices. >> >> Most projects today put project headers first for a reason: it catches the >> frequent case where a header change makes it not self-contained (therefore >> making it possible to break third-party code using that header). >> >> Examples of explicit recommendations to that effect from various >> heavyweights: > > I haven't said I am against this style What I mean is that > spice-server is not following this style, as far as I can tell spice-gtk > is not either, so adding this now to the coding styles is just odd. It > should reflect at least a little bit what is currently being done ;) Well, what is done is discussing style, knowing that the current style is not exceedingly consistent to start with, and in doing so. I pointed out things that are bogus in the existing guidelines. What’s wrong with that? That I’m criticizing your beloved codebase? ;-) > > If we want to change and decide that's important enough, then I would go > with a separate patch series which would change at least part of the > codebase to follow that style, and then add this to the coding style > doc. Though I don't think it's really useful to spend a lot of time on > it... > > Alternatively, this section of the coding style should acknowledge that > this is not followed at all by the current code base, but that new code > should do that, and explain that when making changes to a file, patches > reordering the headers in that file are welcome (if that's how we want > to deal with the change). A comment to that effect was put at the beginning, at Frediano’s suggestion. "This style guide only indicates what we aim to achieve. It does not necessarily reflect the current state of the code.” Ah… But I did not push v4 yet… TBD. Need to check I have not missed some comments. It’s sort of going in all directions, as any discussion about style is bound to. > > Christophe > ___ > 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
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
On Thu, Feb 15, 2018 at 01:14:43PM +0100, Christophe de Dinechin wrote: > > > > On 15 Feb 2018, at 10:19, Christophe Fergeau wrote: > > > > On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote: > >> > >> > >>> On 14 Feb 2018, at 14:35, Christophe Fergeau wrote: > >>> > >>> This one sounds more like an RFC to me > >> > >> Well, this is really a bug fix in the documentation more than a RFC. > >> > >>> , as from a quick look in server/, > >>> this is not the style currently in use. > >> > >> As I pointed out in earlier discussions, this section of the style guide, > >> as written currently, is mostly backwards compared to industry best > >> practices. > >> > >> Most projects today put project headers first for a reason: it catches the > >> frequent case where a header change makes it not self-contained (therefore > >> making it possible to break third-party code using that header). > >> > >> Examples of explicit recommendations to that effect from various > >> heavyweights: > > > > I haven't said I am against this style What I mean is that > > spice-server is not following this style, as far as I can tell spice-gtk > > is not either, so adding this now to the coding styles is just odd. It > > should reflect at least a little bit what is currently being done ;) > > Well, what is done is discussing style, knowing that the current style > is not exceedingly consistent to start with, and in doing so. I > pointed out things that are bogus in the existing guidelines. What’s > wrong with that? That I’m criticizing your beloved codebase? ;-) Even with the smiley, this is not funny nor helping the discussion. Especially when the paragraph you quote starts with "I haven't said I am against this style". All I'm saying is that it's very odd to me to push something to the coding styles which does not match at all what is inconsistently being done in the codebase at the moment. And then I made concrete suggestions as how I would move forward with this patch... Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
> On 15 Feb 2018, at 13:47, Christophe Fergeau wrote: > > On Thu, Feb 15, 2018 at 01:14:43PM +0100, Christophe de Dinechin wrote: >> >> >>> On 15 Feb 2018, at 10:19, Christophe Fergeau wrote: >>> >>> On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote: > On 14 Feb 2018, at 14:35, Christophe Fergeau wrote: > > This one sounds more like an RFC to me Well, this is really a bug fix in the documentation more than a RFC. > , as from a quick look in server/, > this is not the style currently in use. As I pointed out in earlier discussions, this section of the style guide, as written currently, is mostly backwards compared to industry best practices. Most projects today put project headers first for a reason: it catches the frequent case where a header change makes it not self-contained (therefore making it possible to break third-party code using that header). Examples of explicit recommendations to that effect from various heavyweights: >>> >>> I haven't said I am against this style What I mean is that >>> spice-server is not following this style, as far as I can tell spice-gtk >>> is not either, so adding this now to the coding styles is just odd. It >>> should reflect at least a little bit what is currently being done ;) >> Well, what is done is discussing style, knowing that the current style >> is not exceedingly consistent to start with, and in doing so. I >> pointed out things that are bogus in the existing guidelines. What’s >> wrong with that? That I’m criticizing your beloved codebase? ;-) > > Even with the smiley, this is not funny nor helping the discussion. Sorry you feel that way, that wasn’t intentional. I’m having a hard time understanding your pushback, so I’m trying to understand where you are coming from. > Especially when the paragraph you quote starts with "I haven't said I am > against > this style". > All I'm saying is that it's very odd to me to push something to the > coding styles which does not match at all In general, I tried quite hard to follow the existing guidelines, even a few I dislike quite a bit, while offering a valid transition to an applicable C++ style. The only cases where I purposefully deviated, there is a good reason for that, which I tried to explain every time both on list and in the commits. Regarding headers, for example, I showed you the kind of error messages we presently get, which is only one of many reasons I gave. > what is inconsistently being > done in the codebase at the moment. I think you meant “consistently”? if the codebase does it consistently wrong, isn’t the first step to fix the guidelines, and then to fix the code? Or alternatively, explain why I’m wrong to think it’s wrong? > And then I made concrete suggestions > as how I would move forward with this patch… Sorry, I saw your push back, but I did not see the concrete suggestion for moving forward… Would you please be kind enough to rephrase it? Thanks Christophe > > Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers
On Thu, Feb 15, 2018 at 03:25:08PM +0100, Christophe de Dinechin wrote: > > And then I made concrete suggestions > > as how I would move forward with this patch… > > Sorry, I saw your push back, but I did not see the concrete suggestion for > moving forward… Would you please be kind enough to rephrase it? The last 2 paragraphs of https://lists.freedesktop.org/archives/spice-devel/2018-February/041930.html Which you have actually more or less acknowledged by saying you forgot to squash and need to send a v4 or something like that. Given that this specific change does not match at all what we are currently doing in the codebase, I'd emphasize this fact once more in this section, even if it's already mentioned elsewhere in the document. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
