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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
