[PATCH] D119130: [clangd] NFC: Move stdlibg headers handling to Clang

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:56
+// C++ and C Standard Library symbols are considered distinct: e.g. std::printf
+// and ::printf are not treated as the same symbol. The callers have to act
+// accordingly based on available LanguageOptions.
----------------
I'm not sure what "the callers have to act accordingly..." means - can you drop it or elaborate?


================
Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:37
+  static llvm::Optional<Symbol> named(llvm::StringRef Scope,
+                                      llvm::StringRef Name);
+
----------------
kbobyrev wrote:
> kadircet wrote:
> > should scope have trailing `::` ?
> This is consistent with the current behavior; we can probably change it later.
Please document the behavior that's being added here.

(In clangd, "scope" fairly consistently includes "::" - I don't think that convention exists here)


================
Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:66
+  Recognizer();
+  llvm::Optional<Symbol> operator()(const Decl *D);
+
----------------
kbobyrev wrote:
> kadircet wrote:
> > what about macros?
> For now, I'm just moving the code without adding any new capabilities. The only change is in `namespaceSymbols` (to break the dependency on clangd helpers) that Sam pointed out.
Please add a mention of macros to the docs.


================
Comment at: clang/lib/Tooling/Inclusions/StandardLibrary.cpp:119
+        return namespaceSymbols(Parent);
+    return NamespaceSymbols->lookup(D->getQualifiedNameAsString() + "::");
+  }();
----------------
Does this do the right thing for `std::__1::chrono` where `__1` is inline?


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:61
+
+    namespace chrono {}
+
----------------
should there be symbols in this NS and a test for them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130