Is "include what you use" in the coding standards for chromium?

[Prasad Tammana]

I couldn't find "include what you use" in the style guide - http://www.chromium.org/developers/coding-style. Is it part of coding standards for chromium? If it is, I'm curious to know the value it adds.

Thanks,

Prasad

[Gavin Peters (蓋文彼德斯)]

This recently came up for me in a code review, I'm very interested; in a first pass review I pointed out some missing include-what-you-use system libraries, and the other hacker pointed out the benefits to build speed, and declined to include-what-he-uses.

Our lint tool certainly points out include-what-you-use violations, so I'll follow this thread with interest.

I do note that system libraries on my ubuntu system seem to lack #pragma once.  Bummer.

- Gavin

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[James Hawkins]

I don't think iwyu is called out in the style guide yet; however, I have personally been working with the open-source include-what-you-use tool [1] to get it working for Chromium.  There are still a few tweaks that need to be made to the tool to reduce false positives and fix bugs.  The ideal end state is automatic checking of iwyu using the tool run on the bots.

See http://code.google.com/p/include-what-you-use/wiki/WhyIWYU for...why you should iwyu.

[1] http://code.google.com/p/include-what-you-use/

Thanks,

James

--

[Dominic Hamon]

I'd argue the ideal state is running the tool as part of the presubmit, but that depends on how long it takes to run.

[Rachel Blum]

_Not_ including what you use because some other header currently pulls it in creates a fragile build - I'd advise against it. Last time I looked at #include's and #pragma once (in a life before Chrome), all modern compilers track what they include anyways, so a double include carries very little cost. (If you have data saying otherwise, please point me that way - I've last run that test 3 years or so ago)

Rachel

[Peter Kasting]

On Fri, Nov 18, 2011 at 4:42 PM, Gavin Peters (蓋文彼德斯) <gav...@google.com> wrote:

the other hacker pointed out the benefits to build speed, and declined to include-what-he-uses.

This other hacker is flat wrong.  No one on the project should be making this argument.  Ideally we'd have include-what-you-use that would be tool-assisted so it would be easy to do and enforce.  In the interim time to that, intentionally not including what you use is absolutely wrong.  I say this as someone who used to be on the other side of this fence years ago; I was mistaken back then.

The benefit of include-what-you-use is decreased fragility, as Rachel mentioned.

PK 

[John Abd-El-Malek]

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

--

[Viet-Trung Luu]

On Fri, Nov 18, 2011 at 19:12, John Abd-El-Malek <j...@chromium.org> wrote:

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

I would go with this. In fact, I would go further (this may be controversial): If a method Foo() of X requires that it have defined Z (e.g., by including Z's header), then I may use Z in direct connection with X::Foo() without including Z's header.

E.g., if the method is |Z X::Foo()|, I would allow |Z z = x.Foo();| without an explicit inclusion of Z.h. But if I have an independent use of Z elsewhere, I would want to include Z.h.

[Ricardo Vargas]

And to follow that example a little bit more, if class Y requires header Z as part of its interface, and class X implements that interface, the declaration of class X usually don't include Z (or at least that's not what most of our code does). 

[Ryan Sleevi]

On Nov 18, 10:12 pm, John Abd-El-Malek <j...@chromium.org> wrote:
> Just to clarify for myself: if I include class X which derives from class
> Y, I don't have to also include Y's header right?
>

In your example, X *must* include Y.h, since you cannot derive from a
forward-declared base class (the size of the base class must be
known). So your class (B?) should be guaranteed that Y.h is always
included if X.h is included. From what I've seen in the current
codebase, this seems to be consistent assumption.

If X is refactored to no longer be-a Y, then adding Y.h (if needed)
should be part of the normal cleanup involved with breaking the X is-a
Y relationship, since B is a dependent on that is-a relation.

In the context of the larger question, I had assumed that IWYU
naturally followed from the comments on forward declares at
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

Since the preference when defining any interface/implementation is to
forward declare, dependents of anything but an is-a relationship
should be including what they use, since the dependency may be
refactored at any point to forward declare the types, rather than
include their headers.

So if code is calling a method on X, |Z* X::DoFoo()|, then that caller
should be including Z.h in it's implementation file, since X may have
followed (or be made to follow) the C++ Dos and Dont's and just
forward declared Z.

The same would apply for |Z X::DoFoo()| (note: no pointer), since X
can still forward declare Z safely.

So if X can ever forward declare the type (which it can, in many
cases), you should include Z.h. If X cannot forward declare (for
example, it has-a Z member), you should still include Z.h, since
depending on that has-a relationship is an implementation detail, and
not part of the public interface. That's why I thought IWYU was
implied.

[William Chan]

I am for include-what-you-use. I would support using the open source
iwyu tool that James is working on. FWIW, this is also consistent with
internal Google coding practices, which I think is generally good.

[Nico Weber]

On Tue, Nov 22, 2011 at 12:10 PM, William Chan <will...@chromium.org> wrote:
> I am for include-what-you-use. I would support using the open source
> iwyu tool that James is working on. FWIW, this is also consistent with
> internal Google coding practices, which I think is generally good.

We tried it a while ago, and it wasn't ready back then. Maybe that has
changed, but chances are there is some upstream work involved.

Nico

[William Chan (陈智昌)]

Sorry, I meant this as a general, I am in favor of this and would use a tool if it were complete.

[Darin Fisher]

On Fri, Nov 18, 2011 at 7:20 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 19:12, John Abd-El-Malek <j...@chromium.org> wrote:

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

I would go with this. In fact, I would go further (this may be controversial): If a method Foo() of X requires that it have defined Z (e.g., by including Z's header), then I may use Z in direct connection with X::Foo() without including Z's header.

E.g., if the method is |Z X::Foo()|, I would allow |Z z = x.Foo();| without an explicit inclusion of Z.h. But if I have an independent use of Z elsewhere, I would want to include Z.h.

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Maybe I'm just biased since I've always been happy including only what I need.

One pet-peeve I have is when I see someone unnecessarily including build/build_config.h.  It is included by base/basictypes.h, and therefore it is pretty much included into every since .cc file.  It feels like unnecessary verbosity to have to include build/build_config.h everywhere, redundantly.  IWYU would suggest that you should.

-Darin

[Gavin Peters (蓋文彼德斯)]

On 22 November 2011 15:41, Darin Fisher <da...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 7:20 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 19:12, John Abd-El-Malek <j...@chromium.org> wrote:

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

I would go with this. In fact, I would go further (this may be controversial): If a method Foo() of X requires that it have defined Z (e.g., by including Z's header), then I may use Z in direct connection with X::Foo() without including Z's header.

E.g., if the method is |Z X::Foo()|, I would allow |Z z = x.Foo();| without an explicit inclusion of Z.h. But if I have an independent use of Z elsewhere, I would want to include Z.h.

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Darin, it gets worse, I have another exception to enumerate.  Specifically, how does IWYU attach, when overriding, to classes used in the signature.  Example:

a.h:

#include <string>

class A {

 public:

  void foo(std::string& zomg);

};

b.h:

#include "b.h"

class B : public A {

 public:

  void foo(std::string& zomg) OVERRIDE;

};

Does class B in this case IWYU?  The rule stated above about X, Y and Z doesn't specifically address it, but given that B's declaration of foo is a compile-time assertion that depends on A being properly defined, then surely IWYU doesn't attach here, and so we do not need to #include <string> in b.h.

- Gavin

[Wez]

On 22 November 2011 12:41, Darin Fisher <da...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 7:20 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 19:12, John Abd-El-Malek <j...@chromium.org> wrote:

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

I would go with this. In fact, I would go further (this may be controversial): If a method Foo() of X requires that it have defined Z (e.g., by including Z's header), then I may use Z in direct connection with X::Foo() without including Z's header.

E.g., if the method is |Z X::Foo()|, I would allow |Z z = x.Foo();| without an explicit inclusion of Z.h. But if I have an independent use of Z elsewhere, I would want to include Z.h.

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Could the rule be more generally expressed as IWYU except things you're using only as part of an interface of something you're pulling in?

e.g. Code pulling in X, above, but using a method defined by Y doesn't need to explicitly include Y because it's the method it's interested in, not the method.

e.g. Code pulling in X and W, which both derived from Y, and passing them around as Ys, doesn't need to explicitly include Y because if the inheritance relationships change the code will break anyway.

e.g. Code using X:Foo() above doesn't need to include Z, because if X stopped defining Z it's interface would have changed, breaking the calling code anyway.

Wez

[Wez]

On 22 November 2011 12:41, Darin Fisher <da...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 7:20 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 19:12, John Abd-El-Malek <j...@chromium.org> wrote:

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

I would go with this. In fact, I would go further (this may be controversial): If a method Foo() of X requires that it have defined Z (e.g., by including Z's header), then I may use Z in direct connection with X::Foo() without including Z's header.

E.g., if the method is |Z X::Foo()|, I would allow |Z z = x.Foo();| without an explicit inclusion of Z.h. But if I have an independent use of Z elsewhere, I would want to include Z.h.

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Could the rule be more generally expressed as IWYU except things you're only using because they're part of an interface of something you're pulling in?

[James Hawkins]

On Tue, Nov 22, 2011 at 12:41 PM, Darin Fisher <da...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 7:20 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Fri, Nov 18, 2011 at 19:12, John Abd-El-Malek <j...@chromium.org> wrote:

Just to clarify for myself: if I include class X which derives from class Y, I don't have to also include Y's header right?

I would go with this. In fact, I would go further (this may be controversial): If a method Foo() of X requires that it have defined Z (e.g., by including Z's header), then I may use Z in direct connection with X::Foo() without including Z's header.

E.g., if the method is |Z X::Foo()|, I would allow |Z z = x.Foo();| without an explicit inclusion of Z.h. But if I have an independent use of Z elsewhere, I would want to include Z.h.

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Maybe I'm just biased since I've always been happy including only what I need.

One pet-peeve I have is when I see someone unnecessarily including build/build_config.h.  It is included by base/basictypes.h, and therefore it is pretty much included into every since .cc file.  It feels like unnecessary verbosity to have to include build/build_config.h everywhere, redundantly.  IWYU would suggest that you should.

No worries: iwyu has a way to declare (in the source) that base/basictypes.h 'provides' build/build_config.h.

See http://code.google.com/p/include-what-you-use/wiki/WhatIsAUse#Manual_re-export_identifiers

James

[Gavin Peters (蓋文彼德斯)]

n/m the above, it's the same exception that Trung enumerates: I missed that Z isn't an ancestor to Y when I first read trung's post, hence my confusing message above.  Apologies!

- Gavin

- Gavin

[Darin Fisher]

You're making my point for me.  These exceptions to the rule are not trivial to explain.

-Darin

 

- Gavin

- Gavin

[Andrew Scherkus]

I like IWYU but I do agree it's can get complicated.

Until we're at the point where we can rely on presubmits to enforce the rule along with its exceptions I've been going with "when touching #includes try to clean them up if you spot something suspicious".  Same with unused forward declares, friend class statements, using statements, etc...

Andrew

 

-Darin

 

- Gavin

- Gavin

[Viet-Trung Luu]

On Tue, Nov 22, 2011 at 12:41, Darin Fisher <da...@chromium.org> wrote:

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Maybe I'm just biased since I've always been happy including only what I need.

We can argue about the extent and details of the rule, but I think that IWYU is important and prevents fragility. It's quite annoying to remove an include from your header, only to have to track down and fix a bunch of other places which were unwittingly relying on it.

 

One pet-peeve I have is when I see someone unnecessarily including build/build_config.h.  It is included by base/basictypes.h, and therefore it is pretty much included into every since .cc file.  It feels like unnecessary verbosity to have to include build/build_config.h everywhere, redundantly.  IWYU would suggest that you should.

In this particular example, I have to disagree: The (very common)

#if defined(OS_FOO)

...

#endif

construct is exceedingly fragile, and will fail silently if somehow build/build_config.h isn't included. (Arguably, we should replace that construct with something less fragile, though that ship sailed a long time ago.) I'd say that this is even more important in headers that use OS_FOO macros, since they're less likely to include base/basictypes.h.

[Darin Fisher]

On Tue, Nov 22, 2011 at 7:04 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Tue, Nov 22, 2011 at 12:41, Darin Fisher <da...@chromium.org> wrote:

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Maybe I'm just biased since I've always been happy including only what I need.

We can argue about the extent and details of the rule, but I think that IWYU is important and prevents fragility. It's quite annoying to remove an include from your header, only to have to track down and fix a bunch of other places which were unwittingly relying on it.

Yes, I agree that can be annoying.  It certainly isn't very easy to search-n-replace to resolve such errors.  Many try bot runs are often required.  Hmm...

It seems like a similar problem exists with IWYU.  Suppose everyone is happily including X.h, but I want to factor out a portion of X.h to create Y.h.  If the rule is that everyone should exactly include what they use, then I will now need to hunt down everyone who needs Y.h instead of X.h and either add a Y.h include or change the X.h include to a Y.h include.  This seems fairly tedious too.  However, I suppose we could have tools to help us here.

 

 

One pet-peeve I have is when I see someone unnecessarily including build/build_config.h.  It is included by base/basictypes.h, and therefore it is pretty much included into every since .cc file.  It feels like unnecessary verbosity to have to include build/build_config.h everywhere, redundantly.  IWYU would suggest that you should.

In this particular example, I have to disagree: The (very common)

#if defined(OS_FOO)

...

#endif

construct is exceedingly fragile, and will fail silently if somehow build/build_config.h isn't included. (Arguably, we should replace that construct with something less fragile, though that ship sailed a long time ago.) I'd say that this is even more important in headers that use OS_FOO macros, since they're less likely to include base/basictypes.h.

Oh, interesting.  Maybe a better solution is to borrow from WebKit and invent a OS() macro.  Imagine code like:

#if OS(FOO)

...

#endif

The above would fail to build if OS() were undefined.  This seems like an even better way to solve the problem.

-Darin

[James Hawkins]

On Tue, Nov 22, 2011 at 8:43 PM, Darin Fisher <da...@chromium.org> wrote:

On Tue, Nov 22, 2011 at 7:04 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Tue, Nov 22, 2011 at 12:41, Darin Fisher <da...@chromium.org> wrote:

As we enumerate reasonable exceptions to the rule like these, I start to question the value of the rule.  The rule is no longer simple, and it seems like there will just remain a constant din of confusion around this rule.

Maybe I'm just biased since I've always been happy including only what I need.

We can argue about the extent and details of the rule, but I think that IWYU is important and prevents fragility. It's quite annoying to remove an include from your header, only to have to track down and fix a bunch of other places which were unwittingly relying on it.

Yes, I agree that can be annoying.  It certainly isn't very easy to search-n-replace to resolve such errors.  Many try bot runs are often required.  Hmm...

It seems like a similar problem exists with IWYU.  Suppose everyone is happily including X.h, but I want to factor out a portion of X.h to create Y.h.  If the rule is that everyone should exactly include what they use, then I will now need to hunt down everyone who needs Y.h instead of X.h and either add a Y.h include or change the X.h include to a Y.h include.  This seems fairly tedious too.  However, I suppose we could have tools to help us here.

The iwyu tool both warns/errors in one mode and fixes the errors automagically in a second mode.  For doing a simple move/refactor, the tool does a fine job of fixing up the include paths for users of the new class.

[Darin Fisher]

That's quite nifty sounding.

-Darin

[Jói Sigurðsson]

> The iwyu tool both warns/errors in one mode and fixes the errors> automagically in a second mode.  For doing a simple move/refactor, the tool> does a fine job of fixing up the include paths for users of the new class.

James, is the iwyu tool ready for use on Chrome in the latter mode?
For a lot of the moves we're doing for content/ this could be very
useful.

Cheers,
Jói

[James Hawkins]

I'll have to write a script that, given a file that's moved, grep for the classes/types that are used from that file throughout the entire code base; this list of files would then be sent through the iwyu tool.  That shouldn't be too difficult to write; I'll look at it over the US break.

Thanks,

James

[Jói Sigurðsson]

Awesome, thanks James!!

2011/11/23 James Hawkins <jhaw...@chromium.org>:

[Tommi]

Could the iwyu tool be extended to diwydu (don't include what you don't use)?

I often run into code that has seen a bit of changes, but still has #includes for code that once was there.

I've been hunting down global variables recently and while discussing this with Siggi, he pointed out

that merely #including <stdio> introduced a new global variable.  I remember seeing this before although

I don't recall if it was with stdio or not but my gut tells me that there are more evil headers like that

and if we don't use them, it would be nice to get rid of them.

[James Hawkins]

Include-what-you-use implies dont-include-what-you-don't-use, so yes
it already does this.

[Jim Blackler]

Resurrecting this old thread... can anyone tell me if there is an IWYU tool available that works on Chromium today? I found this page https://code.google.com/p/chromium/wiki/IncludeWhatYouUse that says "we stopped looking at it for chromium", but it seems from the archives that people had some results using a tool in the past.

Jim

[Mostyn Bramley-Moore]

After submitting a few "IWYU" patches recently, I created this bug with
some (brief) notes on a couple of utils:
https://code.google.com/p/chromium/issues/detail?id=259043

-Mostyn.

> <jhaw...@chromium.org <javascript:>>

> > wrote:
> >>
> >> I'll have to write a script that, given a file that's moved,
> grep for the
> >> classes/types that are used from that file throughout the entire
> code base;
> >> this list of files would then be sent through the iwyu tool.
> That shouldn't
> >> be too difficult to write; I'll look at it over the US break.
> >>
> >> Thanks,
> >> James
> >>
> >>
> >> On Wed, Nov 23, 2011 at 1:56 AM, J�i Sigur�sson
> <j...@chromium.org <javascript:>> wrote:
> >>>
> >>> > The iwyu tool both warns/errors in one mode and fixes the errors>
> >>> > automagically in a second mode. For doing a simple
> move/refactor, the tool>
> >>> > does a fine job of fixing up the include paths for users of
> the new class.
> >>> James, is the iwyu tool ready for use on Chrome in the latter mode?
> >>> For a lot of the moves we're doing for content/ this could be very
> >>> useful.
> >>>
> >>> Cheers,

> >>> J�i

> >>>
> >>>
> >>> On Wed, Nov 23, 2011 at 4:54 AM, James Hawkins

> <jhaw...@chromium.org <javascript:>>

> >>> wrote:
> >>> >
> >>> > On Tue, Nov 22, 2011 at 8:43 PM, Darin Fisher

> <da...@chromium.org <javascript:>>

> >>> > wrote:
> >>> >>
> >>> >> On Tue, Nov 22, 2011 at 7:04 PM, Viet-Trung Luu
> >>> >> <viettr...@chromium.org <javascript:>> wrote:
> >>> >>>
> >>> >>> On Tue, Nov 22, 2011 at 12:41, Darin Fisher

> <da...@chromium.org <javascript:>>

> <javascript:>

> >>> > View archives, change email options, or unsubscribe:
> >>> > http://groups.google.com/a/chromium.org/group/chromium-dev
> <http://groups.google.com/a/chromium.org/group/chromium-dev>
> >>> >
> >>
> >>
> >> --
> >> Chromium Developers mailing list: chromi...@chromium.org

> <javascript:>

> >> View archives, change email options, or unsubscribe:
> >> http://groups.google.com/a/chromium.org/group/chromium-dev

> <http://groups.google.com/a/chromium.org/group/chromium-dev>
> >
> >
>
> --

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
>


--

Mostyn Bramley-Moore
mos...@opera.com

[Kim Gräsman]

Hi Jim,

On Wed, Aug 7, 2013 at 10:06 AM, Jim Blackler <jimbl...@chromium.org> wrote:
> Resurrecting this old thread... can anyone tell me if there is an IWYU tool
> available that works on Chromium today? I found this page
> https://code.google.com/p/chromium/wiki/IncludeWhatYouUse that says "we
> stopped looking at it for chromium", but it seems from the archives that
> people had some results using a tool in the past.

I can't speak for Chromium, but I'm now one of the maintainers of the
original IWYU tool, available here:
https://code.google.com/p/include-what-you-use/

We'd love to help improve IWYU to work better with Chromium, so let us
know if you run into anything problematic (and with a codebase the
size of Chromium's I'm sure you will :-))

- Kim