[v3,11/11] Rewrite the style guide for headers

Submitted by Christophe de Dinechin on Feb. 8, 2018, 11:25 a.m.

Details

Message ID 20180208112531.19601-12-christophe@dinechin.org
State New
Headers show
Series "Updates to the style guide" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 8, 2018, 11:25 a.m.
From: Christophe de Dinechin <dinechin@redhat.com>

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 <dinechin@redhat.com>
---
 docs/spice_style.txt | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

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 <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]
 ----
-#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 */
 ----
 
 

Comments

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 <dinechin@redhat.com>
> 
> 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 <dinechin@redhat.com>
> ---
>  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 <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]
>  ----
> -#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 */
>  ----
>  
>  
> -- 
> 2.13.5 (Apple Git-94)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@redhat.com> 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 <map>
> 		• 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-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:544:71: error: no template named '__uncvref'; did you mean '::std::__uncvref'?
struct __is_tree_value_type<_One> : __is_tree_value_type_imp<typename __uncvref<_One>::type> {};
                                                                      ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:1121:8: note: '::std::__uncvref' declared here
struct __uncvref  {
       ^
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:564:12: error: no member named 'addressof' in namespace 'spice::std::__1'; did you mean '::std::addressof'?
    return _VSTD::addressof(__n);
           ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:434:1: note: '::std::addressof' declared here
addressof(_Tp& __x) _NOEXCEPT
^
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:570:12: error: no member named 'move' in namespace 'spice::std::__1'; did you mean '::std::move'?
    return _VSTD::move(__v);
           ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:2178:1: note: '::std::move' declared here
move(_Tp&& __t) _NOEXCEPT
^
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:580:11: error: no template named 'pair'; did you mean '::std::pair'?
  typedef pair<const _Key, _Tp>                        __container_value_type;
          ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/utility:309:29: note: '::std::pair' declared here
struct _LIBCPP_TEMPLATE_VIS pair
                            ^
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:581:11: error: no template named 'pair'; did you mean '::std::pair'?
  typedef pair<_Key, _Tp>                              __nc_value_type;
          ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/utility:309:29: note: '::std::pair' declared here
struct _LIBCPP_TEMPLATE_VIS pair
                            ^
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:593:19: error: no template named 'enable_if'; did you mean '::std::enable_if'?
  static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:420:63: note: '::std::enable_if' declared here
template <bool, class _Tp = void> struct _LIBCPP_TEMPLATE_VIS enable_if {};
                                                              ^
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:593:29: error: no template named '__is_same_uncvref'; did you mean '::std::__is_same_uncvref'?
  static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
                            ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:1138:8: note: '::std::__is_same_uncvref' declared here
struct __is_same_uncvref : is_same<typename __uncvref<_Tp>::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:607:19: error: no template named 'enable_if'; did you mean '::std::enable_if'?
  static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:420:63: note: '::std::enable_if' declared here
template <bool, class _Tp = void> struct _LIBCPP_TEMPLATE_VIS enable_if {};
                                                              ^
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:607:29: error: no template named '__is_same_uncvref'; did you mean '::std::__is_same_uncvref'?
  static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
                            ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:1138:8: note: '::std::__is_same_uncvref' declared here
struct __is_same_uncvref : is_same<typename __uncvref<_Tp>::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:615:12: error: no member named 'addressof' in namespace 'spice::std::__1'; did you mean '::std::addressof'?
    return _VSTD::addressof(__n.__cc);
           ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:434:1: note: '::std::addressof' declared here
addressof(_Tp& __x) _NOEXCEPT
^
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:621:12: error: no member named 'move' in namespace 'spice::std::__1'; did you mean '::std::move'?
    return _VSTD::move(__v.__nc);
           ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:2178:1: note: '::std::move' declared here
move(_Tp&& __t) _NOEXCEPT
^
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:631:20: error: no template named '__rebind_pointer'; did you mean '::std::__rebind_pointer'?
  typedef typename __rebind_pointer<_VoidPtr, __node_base_type>::type
                   ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:974:8: note: '::std::__rebind_pointer' declared here
struct __rebind_pointer {
       ^
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:635:20: error: no template named '__rebind_pointer'; did you mean '::std::__rebind_pointer'?
  typedef typename __rebind_pointer<_VoidPtr, __end_node_type>::type
                   ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:974:8: note: '::std::__rebind_pointer' declared here
struct __rebind_pointer {
       ^
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:640:20: error: no template named 'conditional'; did you mean '::std::conditional'?
  typedef typename conditional<
                   ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:409:33: note: '::std::conditional' declared here
    struct _LIBCPP_TEMPLATE_VIS conditional {typedef _If 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:641:7: error: no template named 'is_pointer'; did you mean '::std::is_pointer'?
      is_pointer<__end_node_pointer>::value,
      ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:747:50: note: '::std::is_pointer' declared here
template <class _Tp> struct _LIBCPP_TEMPLATE_VIS is_pointer
                                                 ^
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:647:18: error: no template named 'is_same'; did you mean '::std::is_same'?
  static_assert((is_same<typename pointer_traits<_VoidPtr>::element_type, void>::value),
                 ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:828:61: note: '::std::is_same' declared here
template <class _Tp, class _Up> struct _LIBCPP_TEMPLATE_VIS is_same           : public false_type {};
                                                            ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
—————— 8< ————————————————————————


Thanks
Christophe

> 
> Christophe
> 
> On Thu, Feb 08, 2018 at 12:25:31PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> 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 <dinechin@redhat.com>
>> ---
>> 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 <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]
>> ----
>> -#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 */
>> ----
>> 
>> 
>> -- 
>> 2.13.5 (Apple Git-94)
>> 
>> _______________________________________________
>> 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
On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@redhat.com> 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
On Wed, 2018-02-14 at 23:24 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@redhat.com> 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 <map>
> > 		• 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-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:544:71: error: no template named '__uncvref'; did you mean '::std::__uncvref'?
> struct __is_tree_value_type<_One> : __is_tree_value_type_imp<typename __uncvref<_One>::type> {};
>                                                                       ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:1121:8: note: '::std::__uncvref' declared here
> struct __uncvref  {
>        ^
> 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:564:12: error: no member named 'addressof' in namespace 'spice::std::__1'; did you mean '::std::addressof'?
>     return _VSTD::addressof(__n);
>            ^~~~~~~
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
> #define _VSTD std::_LIBCPP_NAMESPACE
>               ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:434:1: note: '::std::addressof' declared here
> addressof(_Tp& __x) _NOEXCEPT
> ^
> 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:570:12: error: no member named 'move' in namespace 'spice::std::__1'; did you mean '::std::move'?
>     return _VSTD::move(__v);
>            ^~~~~~~
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
> #define _VSTD std::_LIBCPP_NAMESPACE
>               ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:2178:1: note: '::std::move' declared here
> move(_Tp&& __t) _NOEXCEPT
> ^
> 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:580:11: error: no template named 'pair'; did you mean '::std::pair'?
>   typedef pair<const _Key, _Tp>                        __container_value_type;
>           ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/utility:309:29: note: '::std::pair' declared here
> struct _LIBCPP_TEMPLATE_VIS pair
>                             ^
> 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:581:11: error: no template named 'pair'; did you mean '::std::pair'?
>   typedef pair<_Key, _Tp>                              __nc_value_type;
>           ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/utility:309:29: note: '::std::pair' declared here
> struct _LIBCPP_TEMPLATE_VIS pair
>                             ^
> 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:593:19: error: no template named 'enable_if'; did you mean '::std::enable_if'?
>   static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
>                   ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:420:63: note: '::std::enable_if' declared here
> template <bool, class _Tp = void> struct _LIBCPP_TEMPLATE_VIS enable_if {};
>                                                               ^
> 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:593:29: error: no template named '__is_same_uncvref'; did you mean '::std::__is_same_uncvref'?
>   static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
>                             ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:1138:8: note: '::std::__is_same_uncvref' declared here
> struct __is_same_uncvref : is_same<typename __uncvref<_Tp>::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:607:19: error: no template named 'enable_if'; did you mean '::std::enable_if'?
>   static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
>                   ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:420:63: note: '::std::enable_if' declared here
> template <bool, class _Tp = void> struct _LIBCPP_TEMPLATE_VIS enable_if {};
>                                                               ^
> 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:607:29: error: no template named '__is_same_uncvref'; did you mean '::std::__is_same_uncvref'?
>   static typename enable_if<__is_same_uncvref<_Up, __container_value_type>::value,
>                             ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:1138:8: note: '::std::__is_same_uncvref' declared here
> struct __is_same_uncvref : is_same<typename __uncvref<_Tp>::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:615:12: error: no member named 'addressof' in namespace 'spice::std::__1'; did you mean '::std::addressof'?
>     return _VSTD::addressof(__n.__cc);
>            ^~~~~~~
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
> #define _VSTD std::_LIBCPP_NAMESPACE
>               ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:434:1: note: '::std::addressof' declared here
> addressof(_Tp& __x) _NOEXCEPT
> ^
> 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:621:12: error: no member named 'move' in namespace 'spice::std::__1'; did you mean '::std::move'?
>     return _VSTD::move(__v.__nc);
>            ^~~~~~~
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__config:392:15: note: expanded from macro '_VSTD'
> #define _VSTD std::_LIBCPP_NAMESPACE
>               ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:2178:1: note: '::std::move' declared here
> move(_Tp&& __t) _NOEXCEPT
> ^
> 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:631:20: error: no template named '__rebind_pointer'; did you mean '::std::__rebind_pointer'?
>   typedef typename __rebind_pointer<_VoidPtr, __node_base_type>::type
>                    ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:974:8: note: '::std::__rebind_pointer' declared here
> struct __rebind_pointer {
>        ^
> 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:635:20: error: no template named '__rebind_pointer'; did you mean '::std::__rebind_pointer'?
>   typedef typename __rebind_pointer<_VoidPtr, __end_node_type>::type
>                    ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:974:8: note: '::std::__rebind_pointer' declared here
> struct __rebind_pointer {
>        ^
> 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:640:20: error: no template named 'conditional'; did you mean '::std::conditional'?
>   typedef typename conditional<
>                    ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:409:33: note: '::std::conditional' declared here
>     struct _LIBCPP_TEMPLATE_VIS conditional {typedef _If 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:641:7: error: no template named 'is_pointer'; did you mean '::std::is_pointer'?
>       is_pointer<__end_node_pointer>::value,
>       ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:747:50: note: '::std::is_pointer' declared here
> template <class _Tp> struct _LIBCPP_TEMPLATE_VIS is_pointer
>                                                  ^
> 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:647:18: error: no template named 'is_same'; did you mean '::std::is_same'?
>   static_assert((is_same<typename pointer_traits<_VoidPtr>::element_type, void>::value),
>                  ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:828:61: note: '::std::is_same' declared here
> template <class _Tp, class _Up> struct _LIBCPP_TEMPLATE_VIS is_same           : public false_type {};
>                                                             ^
> fatal error: too many errors emitted, stopping now [-ferror-limit=]
> —————— 8< ————————————————————————

That was perhaps too "in your face", but you did make a clear point :)
Even regardless of this error, I also prefer the LLVM header order.

Already have a patch for header reorg in spie-streaming-agent.cpp, so
hoping this makes it in fast :)

Lukas

> Thanks
> Christophe
> 
> > 
> > Christophe
> > 
> > On Thu, Feb 08, 2018 at 12:25:31PM +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > 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 <dinechin@redhat.com>
> > > ---
> > > 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 <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]
> > > ----
> > > -#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 */
> > > ----
> > > 
> > > 
> > > -- 
> > > 2.13.5 (Apple Git-94)
> > > 
> > > _______________________________________________
> > > 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
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 15 Feb 2018, at 10:19, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@redhat.com> 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
On Thu, Feb 15, 2018 at 01:14:43PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 15 Feb 2018, at 10:19, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > 
> > On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote:
> >> 
> >> 
> >>> On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@redhat.com> 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
> On 15 Feb 2018, at 13:47, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Thu, Feb 15, 2018 at 01:14:43PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 15 Feb 2018, at 10:19, Christophe Fergeau <cfergeau@redhat.com> wrote:
>>> 
>>> On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote:
>>>> 
>>>> 
>>>>> On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@redhat.com> 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
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