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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 7 04:44:55 PST 2022

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 {
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...)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list