Re: [PATCH v3 11/11] Rewrite the style guide for headers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2018-02-14 at 23:24 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 14:35, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]