sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:258
   for (const auto *Inc : computeUnusedIncludes(AST)) {
+    // FIXME(kirillbobyrev): Standard library headers are not handled correctly
+    // now because the symbols are typically not directly defined in the
----------------
As discussed offline, I'm not sure this difference in behavior is as temporary 
or as narrow as we originally planned.

I think the reason we want this hack is:
 - umbrella headers need special handling
 - standard library headers are umbrella headers
 - system headers are likely to be standard library headers

But this is complicated by the fact that non-stdlib headers can be umbrella 
headers, system headers are likely to be umbrella headers even if not stdlib, 
stdlib needs different special handling from other umbrella headers, there are 
other headers (non-self-contained) that need special handling, etc.

Among other things, it's going to be hard to just turn this back on.

For now, I'd suggest extracting a function `mayConsiderUnused(const 
Inclusion&)` with the simple logic you have here for now, but we should come up 
with a more concrete plan soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112571

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to