hokein added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:70
QualType Type = E->getBase()->IgnoreImpCasts()->getType();
report(E->getMemberLoc(), resolveType(Type));
return true;
----------------
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).
================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:46
+ Base = Base->getPointeeType();
+ if (const auto *TT = Base->getAs<TypedefType>())
+ return TT->getDecl();
----------------
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'
```
================
Comment at: clang/include/clang/AST/Type.h:2595
template <> const TypedefType *Type::getAs() const;
+template <> const UsingType *Type::getAs() const;
----------------
with the `dyn_cast`, we don't need this template specialization for UsingType
now. But I think it is useful to keep it (I have spent sometime on debugging
out why `getAs<Typedef>` work but `getAs<UsingType>` not).
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
