[PATCH] D112707: [clangd] IncludeCleaner: Be more conservative in marking RecordDecl usage

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

To summarize what I remember from the offline discussion:

- when we have multiple decls, eliminating as many as possible will reduce false negatives.
- reducing false negatives isn't our top priority, so we should focus only on the most impactful/safe cases
- if we see a decl in the main file, we can skip other non-definition decls.
- if we see a record definition anywhere, we can skip all other decls. This can have false positives (opaque types whose definition is incidentally visible) but we think it's hugely impactful. We should be clear that we *only* plan to do this for plain classes, and why.

The last two points could be separate patches, up to you.

Some exceptions that seem relevant:

- we can't skip based on a definition (main file or class) that can't stand alone, usually because of qualifiers. e.g. `void foo::bar() {}`.
- friend declarations are special. I suspect we should conservatively never eliminate based on them.



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:51
+      Result.insert(Definition ? Definition->getLocation()
+                               : RD->getMostRecentDecl()->getLocation());
+      return true;
----------------
kbobyrev wrote:
> kadircet wrote:
> > i think this might turn a direct dependency into a transitive one, e.g. you got forward declarations of `struct Foo;` in a.h and b.h, then c.h includes b.h. In the main file you might have includes for a.h and c.h. Now the most recent redecl happens through c.h hence a.h will be marked as unused, even though it's the one header providing the forward decl directly.
> > 
> > what about just rolling with the definition when it's visible and handling the forward-decl in main file case inside the `add` ? i suppose that's something we'd want for all decls and not just records? it implies passing in main file id and a sourcemanager into the crawler, and inside the `add` before going over all redecls, we just check if most recent decl falls into the main file.
> I was considering that part but I decided it's probably more complications for less benefits but I can see how the false positives might turn out to be a problem.
> 
> I think this is not something we want for all decls for type checking reasons (e.g. functions?). @sammccall and I talked about similar things and I believe this is the conclusion, isn't it?
> 
> Thank you, will do!
Seems like the problem Kadir described still exists if b.h is a definition, you'll mark a.h as unused even though it's the only header providing the symbol directly at all (and maybe you don't need the definition!).

> I think this is not something we want for all decls for type checking reasons (e.g. functions?). @sammccall and I talked about similar things and I believe this is the conclusion, isn't it?

Sorry, I can't really understand what any of the "it"s or "this"s in this sentence refer to :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112707