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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In D112707#3093876 <https://reviews.llvm.org/D112707#3093876>, @sammccall wrote:

> What's the purpose of this patch at a high level? Was it triggered by a real example?
> IIUC it's avoiding false negatives, where we're using a class and an otherwise-unused header forward-declares that class.
> Avoiding false negatives isn't a high priority at this point, unless it's a *really* common case.
> As Kadir says this is subtle and risks introducing false positives.
>
> My inclination is that we shouldn't spend time making to make these heuristics more precise/complicated right now, but I'm willing to be convinced...

LLVM has tons of widely used headers with lots and lots of fowrard-declared classes (mainly AST ones) which result in less-useful unused includes warnings. If I drop the change for the case when definition is not available (or fix by checking whether the last redecl is in the mainfile) then this seems like a clear improvement with no false-positives, WDYT?



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:51
+      Result.insert(Definition ? Definition->getLocation()
+                               : RD->getMostRecentDecl()->getLocation());
+      return true;
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112707