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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140095/new/

https://reviews.llvm.org/D140095