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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Mostly LG, few nits.



================
Comment at: clang-tools-extra/clangd/Headers.h:32
 namespace clang {
+class NamespaceDecl;
 namespace clangd {
----------------
Do we need a forward decl here?


================
Comment at: clang-tools-extra/clangd/Headers.h:42
+public:
+  static llvm::Optional<Header> named(llvm::StringRef);
+
----------------
nit: maybe mention the parameter name? it seems redundant but consistent with what we have around here.


================
Comment at: clang-tools-extra/clangd/Headers.h:72
+  Header header() const;
+  llvm::SmallVector<Header> headers() const;
+
----------------
What's the difference between header() and headers()? Without looking at the code below, I presume this is for symbols that could be defined in multiple headers?


================
Comment at: clang-tools-extra/clangd/Headers.h:330
+  static inline clang::clangd::stdlib::Header getEmptyKey() {
+    return clang::clangd::stdlib::Header(-1);
+  }
----------------
maybe `DenseMapInfo<unsigned>::getEmptyKey()` and `DenseMapInfo<unsigned>::getTombstoneKey()` just like above?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:116
+      Result.User.insert(Redecl->getLocation());
+    if (auto SS = StdRecognizer(D))
+      Result.Stdlib.insert(*SS);
----------------
Maybe pull this upstairs and use early return instead? It's either user code or Stdlib, so maybe add exclusively to one of the sets.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:90
+/// FIXME: remove this hack once the implementation is good enough.
+void setIncludeCleanerAnalyzesStdlib(bool B);
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114077