Header inclusion ordering style addition

Header inclusion ordering style addition

[Patrick Venture]

Each file in each daemon attempts to order the headers in a consistent
way.  Typically, alphabetically.  However, I propose here a more
explicit ordering:

https://gerrit.openbmc-project.xyz/12180

As an example of the ordering:

https://gerrit.openbmc-project.xyz/11421

I feel that having a consistent ordering that deals with the various
types of header files included provides a couple benefits:
* header files are included where you expect to find them
* easily can see that there are c-headers included in a clean group
* defines that the phosphor library headers are treated as normal cpp
libraries and included in that grouping making the headers come across
as system libraries and not something different.

Please take a look.

Patrick

Re: Header inclusion ordering style addition

[James Feist]

On 08/28/2018 10:35 AM, Patrick Venture wrote:
> Each file in each daemon attempts to order the headers in a consistent
> way.  Typically, alphabetically.  However, I propose here a more
> explicit ordering:
> 
> https://gerrit.openbmc-project.xyz/12180
> 
> As an example of the ordering:
> 
> https://gerrit.openbmc-project.xyz/11421
> 

This can be enforced in clang-format with SortIncludes
https://clang.llvm.org/docs/ClangFormatStyleOptions.html and
IncludeCategories regex for specific ordering.

> I feel that having a consistent ordering that deals with the various
> types of header files included provides a couple benefits:
> * header files are included where you expect to find them
> * easily can see that there are c-headers included in a clean group
> * defines that the phosphor library headers are treated as normal cpp
> libraries and included in that grouping making the headers come across
> as system libraries and not something different.
> 
> Please take a look.
> 
> Patrick
> 

Re: Header inclusion ordering style addition

[William Kennington]

As long as this can be done automatically with clang format it sounds
good to me. It seems like the trivial alphabetical ordering built-in
to clang-format works fine for the consistency element. We should get
rid of all local includes and use system includes if the headers get
installed as part of the packaging process.
On Tue, Aug 28, 2018 at 10:54 AM James Feist
<james.feist@linux.intel.com> wrote:
>
> On 08/28/2018 10:35 AM, Patrick Venture wrote:
> > Each file in each daemon attempts to order the headers in a consistent
> > way.  Typically, alphabetically.  However, I propose here a more
> > explicit ordering:
> >
> > https://gerrit.openbmc-project.xyz/12180
> >
> > As an example of the ordering:
> >
> > https://gerrit.openbmc-project.xyz/11421
> >
>
> This can be enforced in clang-format with SortIncludes
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html and
> IncludeCategories regex for specific ordering.
>
> > I feel that having a consistent ordering that deals with the various
> > types of header files included provides a couple benefits:
> > * header files are included where you expect to find them
> > * easily can see that there are c-headers included in a clean group
> > * defines that the phosphor library headers are treated as normal cpp
> > libraries and included in that grouping making the headers come across
> > as system libraries and not something different.
> >
> > Please take a look.
> >
> > Patrick
> >

Re: Header inclusion ordering style addition

[Lei YU]

There is certain case that a header file is expected to be included before
others, e.g. config.h generated by autotool shall be included before other
local header files, to ensure that the configs are correctly defined.
E.g.

    #include "config.h"
    #include "bar.hpp"

Will clang-format re-format the order on the above case? If yes, I have concern
that it could introduce issues, e.g. compile error about undefined symbols.
If clang-format handles it well, it's great!

--
BRs,
Lei YU
On Wed, Aug 29, 2018 at 2:12 AM William Kennington <wak@google.com> wrote:
>
> As long as this can be done automatically with clang format it sounds
> good to me. It seems like the trivial alphabetical ordering built-in
> to clang-format works fine for the consistency element. We should get
> rid of all local includes and use system includes if the headers get
> installed as part of the packaging process.
> On Tue, Aug 28, 2018 at 10:54 AM James Feist
> <james.feist@linux.intel.com> wrote:
> >
> > On 08/28/2018 10:35 AM, Patrick Venture wrote:
> > > Each file in each daemon attempts to order the headers in a consistent
> > > way.  Typically, alphabetically.  However, I propose here a more
> > > explicit ordering:
> > >
> > > https://gerrit.openbmc-project.xyz/12180
> > >
> > > As an example of the ordering:
> > >
> > > https://gerrit.openbmc-project.xyz/11421
> > >
> >
> > This can be enforced in clang-format with SortIncludes
> > https://clang.llvm.org/docs/ClangFormatStyleOptions.html and
> > IncludeCategories regex for specific ordering.
> >
> > > I feel that having a consistent ordering that deals with the various
> > > types of header files included provides a couple benefits:
> > > * header files are included where you expect to find them
> > > * easily can see that there are c-headers included in a clean group
> > > * defines that the phosphor library headers are treated as normal cpp
> > > libraries and included in that grouping making the headers come across
> > > as system libraries and not something different.
> > >
> > > Please take a look.
> > >
> > > Patrick
> > >

RE: Header inclusion ordering style addition

[Tanous, Ed]

> 
> There is certain case that a header file is expected to be included before
> others, e.g. config.h generated by autotool shall be included before other
> local header files, to ensure that the configs are correctly defined.
> E.g.
> 
>     #include "config.h"
>     #include "bar.hpp"
> 
> Will clang-format re-format the order on the above case? If yes, I have
> concern that it could introduce issues, e.g. compile error about undefined
> symbols.
> If clang-format handles it well, it's great!

If clang-format is told to operate this way, it will.  It uses regex matched "priorities" then falls back to alphabetical.  If config.h needs to be first, it just needs to be put in the appropriate spot in the .clang-format file that everyone uses.

I agree with the other sentiments that so long as it's done via clang-format, feel free to impose whatever header order is desired.

RE: Header inclusion ordering style addition

[Tanous, Ed]

I should also mention that bmcweb already uses the clang-format file to order headers based on some specific libraries.  It might be a decent example of how to do it for other repositories.

https://github.com/openbmc/bmcweb/blob/master/.clang-format

-Ed 

Re: Header inclusion ordering style addition

[Patrick Venture]

On Tue, Aug 28, 2018 at 11:51 PM Tanous, Ed <ed.tanous@intel.com> wrote:
>
> >
> > There is certain case that a header file is expected to be included before
> > others, e.g. config.h generated by autotool shall be included before other
> > local header files, to ensure that the configs are correctly defined.
> > E.g.
> >
> >     #include "config.h"
> >     #include "bar.hpp"
> >
> > Will clang-format re-format the order on the above case? If yes, I have
> > concern that it could introduce issues, e.g. compile error about undefined
> > symbols.
> > If clang-format handles it well, it's great!
>
> If clang-format is told to operate this way, it will.  It uses regex matched "priorities" then falls back to alphabetical.  If config.h needs to be first, it just needs to be put in the appropriate spot in the .clang-format file that everyone uses.

You can set a config.h to go ahead of all headers, even the main
source header by setting its priority specifically to -1, since 0 is
used for the main source header.

>
> I agree with the other sentiments that so long as it's done via clang-format, feel free to impose whatever header order is desired.

Re: Header inclusion ordering style addition

[Patrick Venture]

On Tue, Aug 28, 2018 at 11:59 PM Tanous, Ed <ed.tanous@intel.com> wrote:
>
> I should also mention that bmcweb already uses the clang-format file to order headers based on some specific libraries.  It might be a decent example of how to do it for other repositories.
>
> https://github.com/openbmc/bmcweb/blob/master/.clang-format

In this gtest/gmock are put above the other headers, which I think
makes sense for the test code at least, I will grab this explicit line
and add it to the review in progress.

PTAL -- also, there is a review of this clang-format under the docs repo:

https://gerrit.openbmc-project.xyz/12180

>
> -Ed