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

Kirill Bobyrev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 30 01:49:52 PST 2021

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
+  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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list