[PATCH] D140095: [include-cleaner] Fix the member-expr-access usage for sugar type.

[Haojian Wu via Phabricator via cfe-commits]

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