sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG, thanks!
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:45
+ if (Base->isPointerType())
+ Base = Base->getPointeeType();
+ // Unwrap the sugar ElaboratedType.
----------------
I think the relative order of unwrapping pointer and elaboratedtype is correct,
but I also think we're going to end up adding more ignored sugar here and
reasoning about the order is hard.
I think `return getMemberProvider(Base->getPointeeType())` (and the same for
ElaboratedType) is a more robust pattern that will be easier to extend.
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:46
+ Base = Base->getPointeeType();
+ if (const auto *TT = Base->getAs<TypedefType>())
+ return TT->getDecl();
----------------
hokein wrote:
> sammccall wrote:
> > hmm, isn't `getAs<>` wrong and `dyn_cast` right here?
> >
> > e.g. if we have a UsingType wrapping a TypedefType, we'll return the
> > typedef rather than the using (because we check for typedef first, and
> > getAs<> will unwrap)
> oh, right, good catch! Added a test.
>
> I used `getAs` is to unwrap the outer sugar `ElaboratedType` for `Base`, I
> think we have to do it manually here.
>
> ```
> ElaboratedType 0x56313be324a0 'Foo' sugar
> `-UsingType 0x56313be32470 'ns::Foo' sugar
> |-UsingShadow 0x56313be32410 'Foo'
> `-RecordType 0x56313be32200 'struct ns::Foo'
> `-CXXRecord 0x56313be32170 'Foo'
> ```
Yep, unfortunately I think we have to be explicit about what kind of sugar we
want to unwrap vs use.
For written types (i.e. typelocs) we get this for free, as basically want to
unwrap if there's another typeloc (lexically) contained. But I don't know if we
can easily reuse that machinery.
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:70
QualType Type = E->getBase()->IgnoreImpCasts()->getType();
report(E->getMemberLoc(), resolveType(Type));
return true;
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > sammccall wrote:
> > > > only tangentially related, but should these be implicit references?
> > > >
> > > > I don't think we actually want to insert headers based on them, right?
> > > > Just allow people to keep the ones that they have inserted that are
> > > > required for compilation?
> > > >
> > > > This would also make it less severe if our heuristic above goes wrong.
> > > > I don't think we actually want to insert headers based on them, right?
> > >
> > > I think no? We want to insert headers, below is the motivated case
> > >
> > > ```
> > > // all.h
> > > #include "foo.h"
> > > #include "declare.h"
> > >
> > > // declare.h
> > > class Foo;
> > > Foo& getFoo();
> > >
> > > // in main.cc
> > > #include "all.h"
> > > void k() {
> > > getFoo().member;
> > > }
> > > ```
> > >
> > > After the cleanup, we expect: `all.h` is removed, `declare.h`, and
> > > `foo.h` are inserted, right? I think this is the case we should care
> > > about, it is a simple version of protobuf case.
> > I would expect `declare.h` to be removed and `foo.h` to be inserted, and
> > then the user to have to insert `Foo.h`.
> >
> > The problem with inserting `"foo.h"` here is that we'll do it even if
> > `declare.h` includes `foo.h`, which is a false positive according to the
> > IWYS principle.
> >
> > > I think this is the case we should care about, it is a simple version of
> > > protobuf case.
> >
> > I think the plan was to have only limited support for forward-declarations
> > (because we can't reason about whether a full declaration is needed), and
> > consider special-casing protos later because we do understand in that case.
> > I would expect declare.h to be removed and foo.h to be inserted, and then
> > the user to have to insert Foo.h.
>
> I think by "`foo.h` to be inserted" you mean `declare.h`, right? There is a
> problem as well, we break the code after the cleanup :( (I'd image this is a
> common pattern used in LLVM)
>
> Thanks I see the point now. (proto is just a special case rather than doing
> it in generally). I added a FIXME here, and plan to fix it in a follow-up
> patch (this is an existing issue).
> I think by "foo.h to be inserted" you mean declare.h, right
Sorry, yes I meant all.h removed, declare.h inserted. (Not at all what I wrote)
Yes we break the code. The condition for doing so is:
a) the current file is not IWYU-clean to start with
b) the codebase relies on forward-declarations
c) forward-declarations aren't always sufficient
So this is bad, but I'm not sure actually disastrous.
> I added a FIXME here, and plan to fix it in a follow-up patch (this is an
> existing issue).
thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140095/new/
https://reviews.llvm.org/D140095
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
