Open GoogleCodeExporter opened 9 years ago
GoogleCodeExporter
commented
9 years ago Thanks, the example makes sense. It sounds like if we can prove that a method
is an override (with or without C++11 override), we can ignore its arguments in
IWYU analysis. Does that sound about right?
Original comment by kim.gras...@gmail.com on 29 Aug 2013 at 8:14
GoogleCodeExporter
commented
9 years ago I think so (arguments and return type, I guess).Original comment by most...@opera.com on 29 Aug 2013 at 8:25
GoogleCodeExporter
commented
9 years ago I'm a bit worried about covariance for return types, I know we already handle
that, so we just need to make sure we don't short-circuit it.
Anyway, as soon as I have time, I can start from your sample code and see how
it plays out.Original comment by kim.gras...@gmail.com on 29 Aug 2013 at 8:33
GoogleCodeExporter
commented
9 years ago Just to let you know that I'm working on this.
I think this is a variation on the examples listed under Author Intent:
https://code.google.com/p/include-what-you-use/wiki/WhatIsAUse#Author_Intent
I wonder if this is actually a bug: by the reasoning in Wiki article, I feel
like base.h should already re-export all symbols to sub.h.
I'll keep digging.Original comment by kim.gras...@gmail.com on 2 Sep 2013 at 6:28
GoogleCodeExporter
commented
9 years ago Still struggling with this.
Author intent and re-exports don't currently seem to be first-class concepts in
IWYU, and I think they should be. I keep discovering new subtle aspects of this
every time I look.
I _think_ (and I'm not sure) that one significant part of the problem is that
re-exports are only honored one level down. Your example calls for re-exporting
<string> from base.h and base.h from sub.h.
For what it's worth, you can work around this by specifying an export pragma on
the #include <string> in base.h:
#include <string> // IWYU pragma: export
This will force a re-export.
I'll try to get my head around this and come up with a more permanent fix.Original comment by kim.gras...@gmail.com on 20 Sep 2013 at 8:22
GoogleCodeExporter
commented
9 years ago I think I have a solution for this problem (I'm testing with a user-defined
struct instead of std::string, because there are other issues with the latter
on Windows.)
However, I made the mistake of pulling in the latest LLVM/Clang, which broke
everything, and I haven't had time to follow up.
I'll try and post a patch some time this week and we can see whether that
solves your local issues.Original comment by kim.gras...@gmail.com on 3 Oct 2013 at 4:10
GoogleCodeExporter
commented
9 years ago I'm back!
Attached is a raw patch that I *think* will solve this problem, but want to try
the waters to make sure a test case covers all the bases. Mostyn, could you
give this a try and see if it acts as you'd expect?
Volodymyr, I would appreciate your input on the solution as such.
Thanks!Original comment by kim.gras...@gmail.com on 5 Oct 2013 at 3:50
Attachments:
GoogleCodeExporter
commented
9 years ago A small question before I examine the patch more closely: should "override"
affect IWYU behavior? Or the behavior without "override" should be the same as
with it?Original comment by vsap...@gmail.com on 6 Oct 2013 at 8:16
GoogleCodeExporter
commented
9 years ago No, I think the behavior should be the same.
I'm assuming that Clang's handling of overridden methods
(CXXMethodDecl::size_overridden_methods()) works the same with or without
override attribute, and it seems to do so.Original comment by kim.gras...@gmail.com on 7 Oct 2013 at 4:38
GoogleCodeExporter
commented
9 years ago It might take me a couple of days to test the patch.Original comment by most...@opera.com on 7 Oct 2013 at 6:47
GoogleCodeExporter
commented
9 years ago Kim, I think that bug105-raw.patch doesn't really resolve the issue because now
#include <string> is recommended in sub.cc, not sub.h. It is recommended due to
std::string constructor usage in return statement.
As far as I understand, you've implemented solution described in comment #1,
right? But I think that the right approach (also described by you) is to treat
#include <string> in base.h as re-export. In this case sub.h, sub.cc don't need
to #include <string> themselves.Original comment by vsap...@gmail.com on 14 Oct 2013 at 8:56
GoogleCodeExporter
commented
9 years ago Thanks for trying it out!
Yeah, that doesn't sound right. I haven't been able to test with std::string
because I'm on Windows and there are problems with mapping.
I honestly couldn't find a way to re-export explicitly -- I was expecting this
to be manifest in the source, but I wasn't able to isolate the behavior
anywhere.
I wish we could make re-exporting more obvious, because we have several issues
hinging on it at the moment.Original comment by kim.gras...@gmail.com on 20 Oct 2013 at 7:17
GoogleCodeExporter
commented
9 years ago @Kim: as vsapsai mentioned, the patch doesn't seem to work.Original comment by most...@opera.com on 20 Oct 2013 at 9:17
GoogleCodeExporter
commented
9 years ago I had an idea to use transitive desired includes instead of associated desired
includes. Corresponding patch is attached. Please note that it's only proof of
concept and the patch is quite messy. I need more time to think if there are
cases where proposed approach doesn't work. Meanwhile I am open to suggestions
and comments.Original comment by vsap...@gmail.com on 27 Oct 2013 at 3:25
Attachments:
GoogleCodeExporter
commented
9 years ago Thanks for picking this up.
I think it's a healthy direction to move IWYU policy from the RAV to
iwyu_output, that's where all the policy decisions should be, IMO.
I don't fully understand what's going on, though... Do you investigate the
entire #include graph and collect all the headers for which there are uses? And
then from that set do IWYU analysis? Won't that invalidate the analysis?
The change in iwyu.cc is a little suspect. It slows things down enormously and
exposes an unrelated bug for MSVC STL (I'll commit a fix for that shortly.) I'm
guessing it's necessary because you now need use data for the entire graph as
opposed to only the files we report IWYU violations for?
I still don't see how this ties into the base re-export, but it does seem to
solve the problem :-)
It'd be great to hear a little more about the overall idea.Original comment by kim.gras...@gmail.com on 28 Oct 2013 at 8:54
GoogleCodeExporter
commented
9 years ago The main question is why shouldn't IWYU offer to #include <string> in sub.h,
sub.cc. I understand that the reason is the following: sub.h won't compile
without #include "base.h" and base.h requires <string>. I.e. sub.h don't need
to #include <string> directly because it includes it transitively through
"base.h". We already allow transitive includes through associated headers (for
example, usage of std::string in base.cc doesn't trigger recommendation to add
#include <string> in base.cc). So I've decided to apply this mechanism for all
transitive includes.
I've attached a patch which makes "associated desired includes" mechanism more
visible. transitive_desired_includes.patch is basically replacing
AssociatedDesiredIncludes with TransitiveDesiredIncludes and additional changes
to make it work.
> Do you investigate the entire #include graph and collect all the headers for
which there are uses? And then from that set do IWYU analysis? Won't that
invalidate the analysis?
I perform IWYU analysis for each file in #include graph (except of emitting
suggested changes), so I can tell for each file which symbols are used and
which headers are desired. Then in the main file I suppress suggested #includes
if the same #include is already accessed via a chain of desired includes (for
example, we don't need to #include "foo.h", if we need to #include "bar.h" and
bar.h itself needs to #include "foo.h").
I think I've already answered the question "Won't that invalidate the
analysis?" but it led to more thinking. Let's consider IWYU spectrum from
"include everything you use directly" to "include what you use and minimize
number of includes". Offered change moves IWYU closer to the latter point. It
contradicts some of IWYU goals, especially for large codebases. So to some
extent it invalidates analysis. That's why I think it is reasonable to turn
on/off TransitiveDesiredIncludes with a command line argument. By default it is
turned off and current behavior is used.Original comment by vsap...@gmail.com on 1 Nov 2013 at 4:15
Attachments:
GoogleCodeExporter
commented
9 years ago Thanks, now I understand better.
Unconditionally allowing transitive includes seems wrong to me. IWYU has had a
pretty strong policy against that and with good reason, I believe. IWYU is not
strictly a minimization problem, it's just as much a way to make source files
stand on their own, which helps refactoring and reuse.
I'm trying to think of an example of where this is clearly wrong, but I'm
having trouble coming up with one, which may indicate that it's actually a good
idea :-) The discussion on issue 73 is the closest I can find, I think comment
#1 was to the point.
The new patch in comment #16 looks obviously good since it eliminates
duplication. I'm still on the fence regarding the one in comment #14. I'm also
not sure about command-line arguments to alter the behavior -- I feel like
"include what you use and minimize number of includes" is not really what we
want to do, but it's a pragmatic way to work around short-comings.
Maybe we should move this discussion to the mailing list, as it's drifting away
from the issue at hand? I'll send out a related idea there.
Original comment by kim.gras...@gmail.com on 2 Nov 2013 at 4:27
GoogleCodeExporter
commented
9 years ago Here's a refinement on my earlier patch. This works for me as-is in a Linux/GCC
environment, and in an MSVC environment if I change the #include from <string>
to <xstring> (mappings are not in place for MSVC.)
Idealistically, I think the logic is in the wrong place, but practically it's
now solved in the same place as the typedef author-intent special rule. Until
we can find a way to push that to a later phase, I'm fine with keeping it in
RAV.
All tests pass. What do you think?Original comment by kim.gras...@gmail.com on 15 Jan 2014 at 6:02
Attachments:
GoogleCodeExporter
commented
9 years ago Hi, Kim. GetAncestorOfType<CXXMethodDecl> is too broad approach. It gets
triggered in method body too and as the result IWYU recommends to remove
#include "override-include2/bar.h" in
{{{
#include "override-include2/sub.h"
#include "override-include2/bar.h"
IndirectClass sub::foo() {
Bar bar;
IndirectClass ic;
return ic;
}
}}}
So, I think that size_overridden_methods() and HasCovariantReturnType() checks
should be put closer to CXXMethodDecl.Original comment by vsap...@gmail.com on 14 Feb 2014 at 5:49
GoogleCodeExporter
commented
9 years ago Ugh. Thanks, I completely overlooked that.
I did an experiment locally and only ignored type uses if the node was a
descendant of FunctionProtoType. The intent was to only check return type and
argument types, but treat the body as usual.
The problem is the mention of std::string in the return statement will count as
a use and force inclusion of <string>, and we essentially want to black-list
std::string (or in your example, IndirectClass) from being IWYU-analyzed in
this entire file (though there may be exceptions).
Putting this close to CXXMethodDecl won't help (though I think I understand
this well enough to move the check there now), because the body will still be
visited and cause trouble.
Ultimately, we want to detect the override, and then say that for this entire
compilation unit, the return type is exported by base.h as well as sub.h
(indirectly but just as validly.) Unfortunately this seems really hard to do
with IWYU's current design...Original comment by kim.gras...@gmail.com on 15 Feb 2014 at 3:39
GoogleCodeExporter
commented
9 years ago Good point. I haven't realized you need to handle the return type in method
body too.Original comment by vsap...@gmail.com on 15 Feb 2014 at 10:21
Original issue reported on code.google.com by
most...@opera.comon 29 Aug 2013 at 8:05Attachments: