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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks sensible to me, but as I wrote this code we should maybe get a second look (@kadircet?) that it makes sense to lift to clang::tooling.

Some positive experience: we've used it successfully in an internal clang-tidy check.



================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:412
 
-TEST(StdlibTest, All) {
-  auto VectorH = stdlib::Header::named("<vector>");
-  EXPECT_TRUE(VectorH);
-  EXPECT_EQ(llvm::to_string(*VectorH), "<vector>");
-  EXPECT_FALSE(stdlib::Header::named("HeadersTests.cpp"));
-
-  auto Vector = stdlib::Symbol::named("std::", "vector");
-  EXPECT_TRUE(Vector);
-  EXPECT_EQ(llvm::to_string(*Vector), "std::vector");
-  EXPECT_FALSE(stdlib::Symbol::named("std::", "dongle"));
-  EXPECT_FALSE(stdlib::Symbol::named("clang::", "ASTContext"));
-
-  EXPECT_EQ(Vector->header(), *VectorH);
-  EXPECT_THAT(Vector->headers(), ElementsAre(*VectorH));
-}
-
 TEST(StdlibTest, Recognizer) {
   auto TU = TestTU::withCode(R"cpp(
----------------
Recognizer test also needs to be moved


================
Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:1
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/Optional.h"
----------------
This needs a copyright header and a file comment


================
Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:7
+namespace clang {
+namespace stdlib {
+
----------------
namespace tooling { namespace stdlib {


================
Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:13
+// Lightweight class, in fact just an index into a table.
+class Header {
+public:
----------------
what's the design around C vs C++?

e.g. are `std::printf` and `::printf` distinct symbols, are C `printf` and C++ `::printf` distinct symbols, similarly for headers.

(Fine if only C++ is implemented here, but the interface should probably say one way or the other)

In clangd we had a handle on all the callers, but here we have to worry about acquiring callers that e.g. can't easily provide language information.


================
Comment at: clang/lib/Tooling/Inclusions/StandardLibrary.cpp:110
+        return namespaceSymbols(Parent);
+    return NamespaceSymbols->lookup((D->getName() + "::").str());
+  }();
----------------
if D is `std::chrono` I think it just returns "chrono"?

(we should probably have a Recognizer test for this, sorry I left it out...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130