[PATCH] D114077: [clangd] Basic IncludeCleaner support for c/c++ standard library

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Headers.h:32
 namespace clang {
+class NamespaceDecl;
 namespace clangd {
----------------
kbobyrev wrote:
> Do we need a forward decl here?
Decl/NamespaceDecl are needed for the interface of the stdlib symbol recognizer, but we don't need to depend on any of the details of AST here.

Splitting the stdlib stuff into its own header seems possible if you'd prefer that?


================
Comment at: clang-tools-extra/clangd/Headers.h:330
+  static inline clang::clangd::stdlib::Header getEmptyKey() {
+    return clang::clangd::stdlib::Header(-1);
+  }
----------------
kbobyrev wrote:
> maybe `DenseMapInfo<unsigned>::getEmptyKey()` and `DenseMapInfo<unsigned>::getTombstoneKey()` just like above?
empty/tombstone keys are reserved values, and we know what sensible reserved values are better than the traits for unsigned do. 

I can fix the code above if you like.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:90
+/// FIXME: remove this hack once the implementation is good enough.
+void setIncludeCleanerAnalyzesStdlib(bool B);
+
----------------
kbobyrev wrote:
> Not sure but: don't we want a config option instead? We can remove it just as easily since it's all "hidden" right now.
I think we discussed this offline a bunch?

A config option is a bunch more plumbing, and we don't actually want to expose this option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114077