search

HaveF / include-what-you-use

Automatically exported from code.google.com/p/include-what-you-use
Other
0 stars 0 forks source link

Do not require including both a derived class' header and its base #73

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?  Give the *exact* arguments passed
to include-what-you-use, and attach the input source file that gives the
problem (minimal test-cases are much appreciated!)

A.h:
"""
class A {};
"""

B.h:
"""
#include "A.h"
class B : public A {};
"""

test.cpp:
"""
#include "B.h"

int main() {
        A a;
        B b;
        return &a && &b ? 123 : -123;
}
"""

Program execution and output:
"""
$ /tmp/llvm/Release+Asserts/bin/include-what-you-use test.cpp

test.cpp should add these lines:
#include "A.h"                          // for A

test.cpp should remove these lines:

The full include-list for test.cpp:
#include "A.h"                          // for A
#include "B.h"                          // for B

---
"""

What is the expected output? What do you see instead?
The only reason it asks me to include A.h is for the variable of type A.  But 
we already have a variable of type B, and B.h must already include the full 
class declaration of A, because B is derived from A.  This shouldn't be 
considered bootlegging -- there is no possible way that B.h will ever not 
include A.h unless the include hierarchy changes, which in many cases it never 
will.  I think this takes "no bootlegging" too far, and results in lots of 
unnecessary proliferation of header files.

What version of the product are you using? On what operating system?
LLVM r160007, iwyu r357, Ubuntu 12.04 LTS x86.

Please provide any additional information below.

This is something I noticed while running iwyu on Mozilla's codebase.  It would 
ask me to include a lot of clearly pointless header files, like adding 
nsIDOMRange.h when nsRange.h is already included, and the whole point of 
nsRange is to be the class that implements the nsIDOMRange interface.  There 
are lots of examples like this in the Gecko codebase, probably in part because 
we have way too many useless abstract base classes!

Original issue reported on code.google.com by a...@aryeh.name on 11 Jul 2012 at 8:15

GoogleCodeExporter commented 9 years ago 
I don't agree.

"there is no possible way that B.h will ever not include A.h unless the include 
hierarchy changes"

Unfortunately, class hierarchies *do* change. B might change from being a 
subclass of A (is-a) to containing a reference of A (has-a); then B.h does not 
need to include A.h because a forward reference is enough. If test.cpp only 
included B.h, it would suddenly fail to compile.

The point of iwyu is precisely to make code resilient in face of change.

Original comment by rcs...@gmail.com on 11 Jul 2012 at 10:33

GoogleCodeExporter commented 9 years ago 
Okay, yes, it's possible in some cases.  But in some cases the inheritance is 
fundamental to the class' purpose, and using human judgment we can figure that 
it will never change -- or if it does, the changes needed to users would be 
enough that changing a few includes is no big deal.  E.g., I can tell you that 
as long as the classes "nsIDOMNode" and "nsIDOMElement" exist in Mozilla, the 
latter will inherit from the former!

Would it be possible to identify constructs that *require* that one class 
derive from another?  For instance, suppose I did:

  A* a = new B();

or:

  B* b = static_cast<B*>instanceOfA;

Then the file won't compile anyway if the class hierarchy changes.  I suspect 
this would be a large fraction of the cases I'm looking at.  But maybe this 
would be overly complicated to implement, I dunno.

Anyway, I see your point, but also don't think unnecessary includes should be 
viewed as without cost -- even if it's just an aesthetic thing.  At a minimum, 
maybe there should be a way to manually annotate certain includes as "assume 
that this file will always be included here".

Also -- are these redundant includes really free?  Or can it add noticeably to 
compilation time to have to re-include the same large header files many times 
per compilation unit?

Original comment by a...@aryeh.name on 11 Jul 2012 at 11:17

GoogleCodeExporter commented 9 years ago 
> At a minimum, maybe there should be a way to manually 
> annotate certain includes as "assume that this file 
> will always be included here".

You should be able to do this in B.h with IWYU pragma: export.

B.h:
"""
#include "A.h" // IWYU pragma: export
class B : public A {};
"""

test.cpp:
"""
#include "B.h"

int main() {
        A a; // will be satisified by B.h
        B b;
        return &a && &b ? 123 : -123;
}
"""

Hope that helps,
- Kim

Original comment by kim.gras...@gmail.com on 13 Jul 2012 at 6:28

GoogleCodeExporter commented 9 years ago 
Thanks!  I didn't find this in the docs before, but now I see 
<http://code.google.com/p/include-what-you-use/wiki/InstructionsForUsers> links 
to <http://code.google.com/p/include-what-you-use/wiki/IWYUPragmas>, which 
describes how to use these pragmas.  So this is fine by me; the issue can be 
closed as far as I'm concerned.

Original comment by a...@aryeh.name on 15 Jul 2012 at 6:38

GoogleCodeExporter commented 9 years ago 
Yes, I added it last night :-)

Could you verify that it solves the problem before we close the issue?

Thanks!

Original comment by kim.gras...@gmail.com on 15 Jul 2012 at 7:08

GoogleCodeExporter commented 9 years ago 
There are a few cases when IWYU considers indirect includes to be correct. For 
example,
A.h:
"""
class A {};
"""

B.h:
"""
#include "A.h"
class B : public A {
  void foo();
};
"""

B.cpp:
"""
#include "B.h"
void B::foo() {}
"""

And b.cpp has correct #includes/fwd-decls.

So before closing the issue I'd like to look at the code and consider if 
indirect include is acceptable.

Original comment by vsap...@gmail.com on 17 Jul 2012 at 9:07

GoogleCodeExporter commented 9 years ago 
I verify that "// IWYU pragma: export" fixes the issue.  Thanks!

Original comment by a...@aryeh.name on 25 Jul 2012 at 9:20

GoogleCodeExporter commented 9 years ago 
@Volodymyr, the reason IWYU accepts that is because B.h/B.cpp are considered a 
unit -- a header named the same as the .cpp file we're processing is its 
associated header, and it makes sense to treat them as one.

We've discussed running IWYU in different modes before (e.g. issue 77) but I 
think for the current makefile-driven mode, this is the right behavior.

Original comment by kim.gras...@gmail.com on 10 Dec 2012 at 1:29

GoogleCodeExporter commented 9 years ago 
OK, then closing the issue.

Original comment by vsap...@gmail.com on 11 Dec 2012 at 2:36