You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Yan Xu <xu...@apple.com> on 2016/12/14 02:14:18 UTC
Order of includes
For a cpp file foo.cpp, our style guide instructs folks to put the header
foo.hpp at the top of the include list:
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes
This is consistent with Google style guide but in reality most of the our
files follow the rule of "treat foo.hpp the same way as other project
headers".
Since this rule has been introduced to the Mesos style guide I haven't seen
much adoption. Most committers still observe the "old" rule when creating
new files and have been instructing contributors to do the same.
Given the current status I'd suggest we revert the style guide on this?
It's only adding confusion and I don't see the need to do a sweeping change
to the codebase to comply with the new rule.
Thoughts?
Yan
Re: Order of includes
Posted by Yan Xu <xu...@apple.com>.
On Thu, Dec 15, 2016 at 7:44 AM, Michael Park <mp...@apache.org> wrote:
> I would vote to keep the "include yourself first" rule, for reasons that
> Benjamin points out.
>
> I think that we (committers) shouldn't be actively (and silently) going
> against the rules we have in place.
> Aside from that... the constructive thing I can suggest is to help
> enforcement by continuing the work on
> clang-tidy (entirely blocked on me at this point).
>
Would be great if it can fix the existing include order. :)
>
> On Tue, Dec 13, 2016 at 11:15 PM, Benjamin Bannier <
> benjamin.bannier@mesosphere.io> wrote:
>
> > Hi Yan,
> >
> > I don’t feel too strongly about most of our style rules regarding include
> > ordering since they are just about style.
> >
> > > For a cpp file foo.cpp, our style guide instructs folks to put the
> header
> > > foo.hpp at the top of the include list:
> > > https://github.com/apache/mesos/blob/master/docs/c%2B%
> > 2B-style-guide.md#order-of-includes
> > >
> > > This is consistent with Google style guide but in reality most of the
> our
> > > files follow the rule of "treat foo.hpp the same way as other project
> > > headers”.
> >
> > Among all our style rules regarding includes, this one actually does have
> > a solid technical justification: It helps ensure that a header file
> always
> > includes all symbols it requires (OK, possibly via discouraged transitive
> > includes in the header itself). Not strictly following this rule has lead
> > to broken header files making their way into the code base (both in the
> > case of internal and public headers), see e.g.,
> >
> > https://reviews.apache.org/r/54083/
> > https://reviews.apache.org/r/54084/
> > https://reviews.apache.org/r/54083/
> >
> > I’d rather have us follow a style that performs some automagic checking
> of
> > header completeness than rely on humans to catch all issues.
> >
> > Note that including `foo.hpp` first in `foo.cpp` is common practice, and
> I
> > expect following this rule would lead to _less friction_ for newcomers to
> > the Mesos code base, see e.g., (no particular order)
> >
> > http://llvm.org/docs/CodingStandards.html#include-style
> > https://github.com/bloomberg/bde/wiki/physical-code-
> > organization#component-design-rules
> > https://webkit.org/code-style-guidelines/#include-statements
> > https://github.com/facebook/hhvm/blob/master/hphp/doc/
> > coding-conventions.md#what-to-include
> > https://google.github.io/styleguide/cppguide.html#
> > Names_and_Order_of_Includes
> >
> >
> > Cheers,
> >
> > Benjamin
>
Re: Order of includes
Posted by Michael Park <mp...@apache.org>.
I would vote to keep the "include yourself first" rule, for reasons that
Benjamin points out.
I think that we (committers) shouldn't be actively (and silently) going
against the rules we have in place.
Aside from that... the constructive thing I can suggest is to help
enforcement by continuing the work on
clang-tidy (entirely blocked on me at this point).
On Tue, Dec 13, 2016 at 11:15 PM, Benjamin Bannier <
benjamin.bannier@mesosphere.io> wrote:
> Hi Yan,
>
> I don’t feel too strongly about most of our style rules regarding include
> ordering since they are just about style.
>
> > For a cpp file foo.cpp, our style guide instructs folks to put the header
> > foo.hpp at the top of the include list:
> > https://github.com/apache/mesos/blob/master/docs/c%2B%
> 2B-style-guide.md#order-of-includes
> >
> > This is consistent with Google style guide but in reality most of the our
> > files follow the rule of "treat foo.hpp the same way as other project
> > headers”.
>
> Among all our style rules regarding includes, this one actually does have
> a solid technical justification: It helps ensure that a header file always
> includes all symbols it requires (OK, possibly via discouraged transitive
> includes in the header itself). Not strictly following this rule has lead
> to broken header files making their way into the code base (both in the
> case of internal and public headers), see e.g.,
>
> https://reviews.apache.org/r/54083/
> https://reviews.apache.org/r/54084/
> https://reviews.apache.org/r/54083/
>
> I’d rather have us follow a style that performs some automagic checking of
> header completeness than rely on humans to catch all issues.
>
> Note that including `foo.hpp` first in `foo.cpp` is common practice, and I
> expect following this rule would lead to _less friction_ for newcomers to
> the Mesos code base, see e.g., (no particular order)
>
> http://llvm.org/docs/CodingStandards.html#include-style
> https://github.com/bloomberg/bde/wiki/physical-code-
> organization#component-design-rules
> https://webkit.org/code-style-guidelines/#include-statements
> https://github.com/facebook/hhvm/blob/master/hphp/doc/
> coding-conventions.md#what-to-include
> https://google.github.io/styleguide/cppguide.html#
> Names_and_Order_of_Includes
>
>
> Cheers,
>
> Benjamin
Re: Order of includes
Posted by Vinod Kone <vi...@apache.org>.
@Mpark, @Yan and @Benjamin: Would one of you be willing to send a review
request that sweeps our code base to enforce this change? It's confusing if
we have some files following the rule and some that doesn't.
On Wed, Dec 21, 2016 at 10:11 PM, Alex Rukletsov <al...@mesosphere.com>
wrote:
> Yes!
>
> https://issues.apache.org/jira/browse/MESOS-6827
>
> On Mon, Dec 19, 2016 at 8:49 AM, Yan Xu <xu...@apple.com> wrote:
>
> > The example is helpful. Thanks!
> >
> > I have no objection to sticking to the new rule then. But the we have to:
> >
> > - For contributors and committers, start using the new style when
> creating
> > new files today.
> > - Fix the existing include order hopefully with the help of tools like
> > clang-tidy and have it enforce the style going forward.
> >
> > Agreed?
> >
> > ---
> > @xujyan <https://twitter.com/xujyan>
> >
> > On Fri, Dec 16, 2016 at 8:54 PM, Benjamin Bannier <
> > benjamin.bannier@mesosphere.io> wrote:
> >
> > > Hi,
> > >
> > > > How does putting your own header at the top (vs. ~the bottom) help
> > ensure
> > > > "a header file always includes all symbols it requires”?
> > >
> > >
> > > Given an incomplete header
> > >
> > > // foo.hpp
> > > std::string f();
> > >
> > > // foo.cpp
> > > #include “foo.hpp”
> > > #include <string>
> > >
> > > std::string f() { return {}; }
> > >
> > > I get
> > >
> > > % clang++ -fsyntax-only foo.cpp --std=c++11
> > > In file included from foo.cpp:1:
> > > ./foo.hpp:1:1: error: use of undeclared identifier 'std'
> > > std::string f();
> > > ^
> > > 1 error generated.
> > >
> > > Swapping the include order makes this pass as `#include` is just
> textual
> > > replacement, and the `#include <string>` in `foo.cpp` would declare the
> > > symbol used in `foo.hpp`.
> > >
> > >
> > > Cheers,
> > >
> > > Benjamin
> >
>
Re: Order of includes
Posted by Alex Rukletsov <al...@mesosphere.com>.
Yes!
https://issues.apache.org/jira/browse/MESOS-6827
On Mon, Dec 19, 2016 at 8:49 AM, Yan Xu <xu...@apple.com> wrote:
> The example is helpful. Thanks!
>
> I have no objection to sticking to the new rule then. But the we have to:
>
> - For contributors and committers, start using the new style when creating
> new files today.
> - Fix the existing include order hopefully with the help of tools like
> clang-tidy and have it enforce the style going forward.
>
> Agreed?
>
> ---
> @xujyan <https://twitter.com/xujyan>
>
> On Fri, Dec 16, 2016 at 8:54 PM, Benjamin Bannier <
> benjamin.bannier@mesosphere.io> wrote:
>
> > Hi,
> >
> > > How does putting your own header at the top (vs. ~the bottom) help
> ensure
> > > "a header file always includes all symbols it requires”?
> >
> >
> > Given an incomplete header
> >
> > // foo.hpp
> > std::string f();
> >
> > // foo.cpp
> > #include “foo.hpp”
> > #include <string>
> >
> > std::string f() { return {}; }
> >
> > I get
> >
> > % clang++ -fsyntax-only foo.cpp --std=c++11
> > In file included from foo.cpp:1:
> > ./foo.hpp:1:1: error: use of undeclared identifier 'std'
> > std::string f();
> > ^
> > 1 error generated.
> >
> > Swapping the include order makes this pass as `#include` is just textual
> > replacement, and the `#include <string>` in `foo.cpp` would declare the
> > symbol used in `foo.hpp`.
> >
> >
> > Cheers,
> >
> > Benjamin
>
Re: Order of includes
Posted by Yan Xu <xu...@apple.com>.
The example is helpful. Thanks!
I have no objection to sticking to the new rule then. But the we have to:
- For contributors and committers, start using the new style when creating
new files today.
- Fix the existing include order hopefully with the help of tools like
clang-tidy and have it enforce the style going forward.
Agreed?
---
@xujyan <https://twitter.com/xujyan>
On Fri, Dec 16, 2016 at 8:54 PM, Benjamin Bannier <
benjamin.bannier@mesosphere.io> wrote:
> Hi,
>
> > How does putting your own header at the top (vs. ~the bottom) help ensure
> > "a header file always includes all symbols it requires”?
>
>
> Given an incomplete header
>
> // foo.hpp
> std::string f();
>
> // foo.cpp
> #include “foo.hpp”
> #include <string>
>
> std::string f() { return {}; }
>
> I get
>
> % clang++ -fsyntax-only foo.cpp --std=c++11
> In file included from foo.cpp:1:
> ./foo.hpp:1:1: error: use of undeclared identifier 'std'
> std::string f();
> ^
> 1 error generated.
>
> Swapping the include order makes this pass as `#include` is just textual
> replacement, and the `#include <string>` in `foo.cpp` would declare the
> symbol used in `foo.hpp`.
>
>
> Cheers,
>
> Benjamin
Re: Order of includes
Posted by Benjamin Bannier <be...@mesosphere.io>.
Hi,
> How does putting your own header at the top (vs. ~the bottom) help ensure
> "a header file always includes all symbols it requires”?
Given an incomplete header
// foo.hpp
std::string f();
// foo.cpp
#include “foo.hpp”
#include <string>
std::string f() { return {}; }
I get
% clang++ -fsyntax-only foo.cpp --std=c++11
In file included from foo.cpp:1:
./foo.hpp:1:1: error: use of undeclared identifier 'std'
std::string f();
^
1 error generated.
Swapping the include order makes this pass as `#include` is just textual replacement, and the `#include <string>` in `foo.cpp` would declare the symbol used in `foo.hpp`.
Cheers,
Benjamin
Re: Order of includes
Posted by Yan Xu <xu...@apple.com>.
On Wed, Dec 14, 2016 at 3:15 PM, Benjamin Bannier <benjamin.bannier
@mesosphere.io> wrote:
> Hi Yan,
>
> I don’t feel too strongly about most of our style rules regarding include
> ordering since they are just about style.
>
> > For a cpp file foo.cpp, our style guide instructs folks to put the header
> > foo.hpp at the top of the include list:
> > https://github.com/apache/mesos/blob/master/docs/c%2B%2B-
> style-guide.md#order-of-includes
> >
> > This is consistent with Google style guide but in reality most of the our
> > files follow the rule of "treat foo.hpp the same way as other project
> > headers”.
>
> Among all our style rules regarding includes, this one actually does have
> a solid technical justification: It helps ensure that a header file always
> includes all symbols it requires (OK, possibly via discouraged transitive
> includes in the header itself). Not strictly following this rule has lead
> to broken header files making their way into the code base (both in the
> case of internal and public headers), see e.g.,
>
> https://reviews.apache.org/r/54083/
> https://reviews.apache.org/r/54084/
> https://reviews.apache.org/r/54083/
How does putting your own header at the top (vs. ~the bottom) help ensure
"a header file always includes all symbols it requires"?
>
>
> I’d rather have us follow a style that performs some automagic checking of
> header completeness than rely on humans to catch all issues.
>
> Note that including `foo.hpp` first in `foo.cpp` is common practice, and I
> expect following this rule would lead to _less friction_ for newcomers to
> the Mesos code base, see e.g., (no particular order)
>
> http://llvm.org/docs/CodingStandards.html#include-style
> https://github.com/bloomberg/bde/wiki/physical-code-organiza
> tion#component-design-rules
> https://webkit.org/code-style-guidelines/#include-statements
> https://github.com/facebook/hhvm/blob/master/hphp/doc/coding
> -conventions.md#what-to-include
> https://google.github.io/styleguide/cppguide.html#Names_and_
> Order_of_Includes
>
>
>
Yeah I have no issues with this practice. I am mainly commenting on the
status quo: there's an entrenched practice and the effort to change it
seemed to have gone no where. I don't personally like one approach better
than the other so I'd like to hear the community thoughts (e.g., technical
benefit) on this and hope we reach a consensus.
Cheers,
>
> Benjamin
Re: Order of includes
Posted by Benjamin Bannier <be...@mesosphere.io>.
Hi Yan,
I don’t feel too strongly about most of our style rules regarding include ordering since they are just about style.
> For a cpp file foo.cpp, our style guide instructs folks to put the header
> foo.hpp at the top of the include list:
> https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes
>
> This is consistent with Google style guide but in reality most of the our
> files follow the rule of "treat foo.hpp the same way as other project
> headers”.
Among all our style rules regarding includes, this one actually does have a solid technical justification: It helps ensure that a header file always includes all symbols it requires (OK, possibly via discouraged transitive includes in the header itself). Not strictly following this rule has lead to broken header files making their way into the code base (both in the case of internal and public headers), see e.g.,
https://reviews.apache.org/r/54083/
https://reviews.apache.org/r/54084/
https://reviews.apache.org/r/54083/
I’d rather have us follow a style that performs some automagic checking of header completeness than rely on humans to catch all issues.
Note that including `foo.hpp` first in `foo.cpp` is common practice, and I expect following this rule would lead to _less friction_ for newcomers to the Mesos code base, see e.g., (no particular order)
http://llvm.org/docs/CodingStandards.html#include-style
https://github.com/bloomberg/bde/wiki/physical-code-organization#component-design-rules
https://webkit.org/code-style-guidelines/#include-statements
https://github.com/facebook/hhvm/blob/master/hphp/doc/coding-conventions.md#what-to-include
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
Cheers,
Benjamin
Re: Order of includes
Posted by haosdent <ha...@gmail.com>.
> "treat foo.hpp the same way as other project headers".
+1 This one may more easily to follow.
> 1) If you rely on symbols from bar.h, don't count on the fact that you included
foo.h which (currently) includes bar.h: include bar.h yourself,
> 2) unless foo.h explicitly demonstrates its intent to provide you the symbols
of bar.h.
> 3) However, any includes present in the related header do not need to be included
again in the related cc (i.e., foo.cc can rely on foo.h's includes).
I think most contributors have followed this for a long time?
+1 for "clarify this in the Mesos style guide"
On Wed, Dec 14, 2016 at 10:29 AM, Yan Xu <xu...@apple.com> wrote:
> Another related practice we should standardize is that, Google style guide
> suggests here
> <https://google.github.io/styleguide/cppguide.html#
> Names_and_Order_of_Includes>
> :
>
> 1) If you rely on symbols from bar.h, don't count on the fact that you
> included foo.h which (currently) includes bar.h: include bar.h yourself,
> 2) unless foo.h explicitly demonstrates its intent to provide you the
> symbols of bar.h.
> 3) However, any includes present in the related header do not need to be
> included again in the related cc (i.e., foo.cc can rely on foo.h's
> includes).
>
> We obviously follow 1) but AFAIK we don't follow 2) and 3), instead we have
> been doing "include everything needed yourself, no exceptions".
>
> It would be great to clarify this in the Mesos style guide as well.
>
> ---
> @xujyan <https://twitter.com/xujyan>
>
> On Wed, Dec 14, 2016 at 10:14 AM, Yan Xu <xu...@apple.com> wrote:
>
> > For a cpp file foo.cpp, our style guide instructs folks to put the header
> > foo.hpp at the top of the include list: https://github.com/apache/meso
> > s/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes
> >
> > This is consistent with Google style guide but in reality most of the our
> > files follow the rule of "treat foo.hpp the same way as other project
> > headers".
> >
> > Since this rule has been introduced to the Mesos style guide I haven't
> > seen much adoption. Most committers still observe the "old" rule when
> > creating new files and have been instructing contributors to do the same.
> >
> > Given the current status I'd suggest we revert the style guide on this?
> > It's only adding confusion and I don't see the need to do a sweeping
> change
> > to the codebase to comply with the new rule.
> >
> > Thoughts?
> >
> > Yan
> >
>
--
Best Regards,
Haosdent Huang
Re: Order of includes
Posted by Yan Xu <xu...@apple.com>.
Another related practice we should standardize is that, Google style guide
suggests here
<https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes>
:
1) If you rely on symbols from bar.h, don't count on the fact that you
included foo.h which (currently) includes bar.h: include bar.h yourself,
2) unless foo.h explicitly demonstrates its intent to provide you the
symbols of bar.h.
3) However, any includes present in the related header do not need to be
included again in the related cc (i.e., foo.cc can rely on foo.h's
includes).
We obviously follow 1) but AFAIK we don't follow 2) and 3), instead we have
been doing "include everything needed yourself, no exceptions".
It would be great to clarify this in the Mesos style guide as well.
---
@xujyan <https://twitter.com/xujyan>
On Wed, Dec 14, 2016 at 10:14 AM, Yan Xu <xu...@apple.com> wrote:
> For a cpp file foo.cpp, our style guide instructs folks to put the header
> foo.hpp at the top of the include list: https://github.com/apache/meso
> s/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes
>
> This is consistent with Google style guide but in reality most of the our
> files follow the rule of "treat foo.hpp the same way as other project
> headers".
>
> Since this rule has been introduced to the Mesos style guide I haven't
> seen much adoption. Most committers still observe the "old" rule when
> creating new files and have been instructing contributors to do the same.
>
> Given the current status I'd suggest we revert the style guide on this?
> It's only adding confusion and I don't see the need to do a sweeping change
> to the codebase to comply with the new rule.
>
> Thoughts?
>
> Yan
>