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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 406794.
kbobyrev added a comment.

Add a comment for the helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

Files:
  clang-tools-extra/clangd/CSymbolMap.inc
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/StdSymbolMap.inc
  clang-tools-extra/clangd/include-mapping/cppreference_parser.py
  clang-tools-extra/clangd/include-mapping/gen_std.py
  clang-tools-extra/clangd/include-mapping/test.py
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
  clang/include/clang/Tooling/Inclusions/CSymbolMap.inc
  clang/include/clang/Tooling/Inclusions/StandardLibrary.h
  clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc
  clang/lib/Tooling/Inclusions/CMakeLists.txt
  clang/lib/Tooling/Inclusions/StandardLibrary.cpp
  clang/tools/include-mapping/cppreference_parser.py
  clang/tools/include-mapping/gen_std.py
  clang/tools/include-mapping/test.py
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/StandardLibraryTest.cpp

Index: clang/unittests/Tooling/StandardLibraryTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/StandardLibraryTest.cpp
@@ -0,0 +1,106 @@
+//===- unittest/Tooling/StandardLibrary.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Inclusions/StandardLibrary.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclarationName.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ScopedPrinter.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using ::testing::ElementsAre;
+
+namespace clang {
+namespace tooling {
+namespace {
+
+const NamedDecl &lookup(ASTUnit &AST, llvm::StringRef Name) {
+  auto &Ctx = AST.getASTContext();
+  TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+  auto Result = TU->lookup(DeclarationName(&Ctx.Idents.get(Name)));
+  assert(!Result.empty() && "Lookup failed");
+  assert(Result.isSingleResult() && "Lookup returned multiple results");
+  return *Result.front();
+}
+
+TEST(StdlibTest, All) {
+  auto VectorH = stdlib::Header::named("");
+  EXPECT_TRUE(VectorH);
+  EXPECT_EQ(llvm::to_string(*VectorH), "");
+  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) {
+  std::unique_ptr AST = buildASTFromCode(R"cpp(
+namespace std {
+inline namespace inl {
+
+template 
+struct vector { class nested {}; };
+
+class secret {};
+
+} // inl
+
+inline namespace __1 {
+  namespace chrono {
+inline namespace chrono_inl {
+class system_clock {};
+} // chrono_inl
+  } // chrono
+} // __1
+
+} // std
+
+class vector {};
+std::vector vec;
+std::vector::nested nest;
+std::secret sec;
+std::chrono::system_clock clock;
+  )cpp");
+
+  auto &VectorNonstd = lookup(*AST, "vector");
+  auto *Vec =
+  cast(lookup(*AST, "vec")).getType()->getAsCXXRecordDecl();
+  auto *Nest =
+  cast(lookup(*AST, "nest")).getType()->getAsCXXRecordDecl();
+  auto *Sec =
+  cast(lookup(*AST, "sec")).getType()->getAsCXXRecordDecl();
+
+  stdlib::Recognizer Recognizer;
+
+  EXPECT_EQ(Recognizer(&VectorNonstd), llvm::None);
+  EXPECT_EQ(Recognizer(Vec), stdlib::Symbol::named("std::", "vector"));
+  EXPECT_EQ(Recognizer(Nest), stdlib::Symbol::named("std::", "vector"));
+  EXPECT_EQ(Recognizer(Sec), llvm::None);
+
+  auto *Chrono =
+  llvm::dyn_cast(cast(lookup(*AST, "clock"))
+.getType()
+->getAsCXXRecordDecl()
+->getParent());
+  EXPECT_EQ(stdlib::getScope(Chrono), "chrono::std::");
+}
+
+} // namespace
+} // namespace tooling
+} // namespace clang
Index: clang/unittests/Tooling/CMakeLists.txt
===
--- clang/unittests/Tooling/CM

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 406793.
kbobyrev marked 5 inline comments as done.
kbobyrev added a comment.

Address the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

Files:
  clang-tools-extra/clangd/CSymbolMap.inc
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/StdSymbolMap.inc
  clang-tools-extra/clangd/include-mapping/cppreference_parser.py
  clang-tools-extra/clangd/include-mapping/gen_std.py
  clang-tools-extra/clangd/include-mapping/test.py
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
  clang/include/clang/Tooling/Inclusions/CSymbolMap.inc
  clang/include/clang/Tooling/Inclusions/StandardLibrary.h
  clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc
  clang/lib/Tooling/Inclusions/CMakeLists.txt
  clang/lib/Tooling/Inclusions/StandardLibrary.cpp
  clang/tools/include-mapping/cppreference_parser.py
  clang/tools/include-mapping/gen_std.py
  clang/tools/include-mapping/test.py
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/StandardLibraryTest.cpp

Index: clang/unittests/Tooling/StandardLibraryTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/StandardLibraryTest.cpp
@@ -0,0 +1,106 @@
+//===- unittest/Tooling/StandardLibrary.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Inclusions/StandardLibrary.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclarationName.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ScopedPrinter.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using ::testing::ElementsAre;
+
+namespace clang {
+namespace tooling {
+namespace {
+
+const NamedDecl &lookup(ASTUnit &AST, llvm::StringRef Name) {
+  auto &Ctx = AST.getASTContext();
+  TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+  auto Result = TU->lookup(DeclarationName(&Ctx.Idents.get(Name)));
+  assert(!Result.empty() && "Lookup failed");
+  assert(Result.isSingleResult() && "Lookup returned multiple results");
+  return *Result.front();
+}
+
+TEST(StdlibTest, All) {
+  auto VectorH = stdlib::Header::named("");
+  EXPECT_TRUE(VectorH);
+  EXPECT_EQ(llvm::to_string(*VectorH), "");
+  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) {
+  std::unique_ptr AST = buildASTFromCode(R"cpp(
+namespace std {
+inline namespace inl {
+
+template 
+struct vector { class nested {}; };
+
+class secret {};
+
+} // inl
+
+inline namespace __1 {
+  namespace chrono {
+inline namespace chrono_inl {
+class system_clock {};
+} // chrono_inl
+  } // chrono
+} // __1
+
+} // std
+
+class vector {};
+std::vector vec;
+std::vector::nested nest;
+std::secret sec;
+std::chrono::system_clock clock;
+  )cpp");
+
+  auto &VectorNonstd = lookup(*AST, "vector");
+  auto *Vec =
+  cast(lookup(*AST, "vec")).getType()->getAsCXXRecordDecl();
+  auto *Nest =
+  cast(lookup(*AST, "nest")).getType()->getAsCXXRecordDecl();
+  auto *Sec =
+  cast(lookup(*AST, "sec")).getType()->getAsCXXRecordDecl();
+
+  stdlib::Recognizer Recognizer;
+
+  EXPECT_EQ(Recognizer(&VectorNonstd), llvm::None);
+  EXPECT_EQ(Recognizer(Vec), stdlib::Symbol::named("std::", "vector"));
+  EXPECT_EQ(Recognizer(Nest), stdlib::Symbol::named("std::", "vector"));
+  EXPECT_EQ(Recognizer(Sec), llvm::None);
+
+  auto *Chrono =
+  llvm::dyn_cast(cast(lookup(*AST, "clock"))
+.getType()
+->getAsCXXRecordDecl()
+->getParent());
+  EXPECT_EQ(stdlib::getScope(Chrono), "chrono::std::");
+}
+
+} // namespace
+} // namespace tooling
+} // namespace clang
Index: clang/unittests/Tooling/CMakeLists.txt
===
--- 

[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 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 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In D119130#3300780 , @kadircet wrote:

> Regarding the include mapping generator, I think it would've been better if 
> we had some sort of list directly from libc++ (as this is now being part of 
> clang rather than just clangd), but having the current symbol mapping 
> available for other tools too is definitely a useful improvement and 
> implementation details can change later on.
> I think we should have some "more public" documentation available around the 
> limitations of current generator though, so that people are not surprised and 
> aware of the caveats (like symbols might be missing, or in case of ambiguity 
> they might be dropped, etc). Not sure where that belongs though, maybe header 
> of the `.inc` file, or if we want it to be only used through the recognizer 
> interface, maybe we can make the `inc` file "private" and document it there.
>
> As for stdlib symbol/header/recognizer, I've got a couple questions around 
> the functionality we are exposing:
>
> - (briefly mentioned above) should we just make raw symbol mappings an 
> implementation detail of stdlib recognizer and have applications depend on it?
> - do we want headers/symbols to be created outside of the recognizer?

Right, I agree that using libc++ directly would be better, but for now this 
just makes the functionality public. We can change it and update as we want 
afterwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:37
+  static llvm::Optional named(llvm::StringRef Scope,
+  llvm::StringRef Name);
+

kadircet wrote:
> should scope have trailing `::` ?
This is consistent with the current behavior; we can probably change it later.



Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:66
+  Recognizer();
+  llvm::Optional operator()(const Decl *D);
+

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 406741.
kbobyrev marked 9 inline comments as done.
kbobyrev added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

Files:
  clang-tools-extra/clangd/CSymbolMap.inc
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/StdSymbolMap.inc
  clang-tools-extra/clangd/include-mapping/cppreference_parser.py
  clang-tools-extra/clangd/include-mapping/gen_std.py
  clang-tools-extra/clangd/include-mapping/test.py
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
  clang/include/clang/Tooling/Inclusions/CSymbolMap.inc
  clang/include/clang/Tooling/Inclusions/StandardLibrary.h
  clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc
  clang/lib/Tooling/Inclusions/CMakeLists.txt
  clang/lib/Tooling/Inclusions/StandardLibrary.cpp
  clang/tools/include-mapping/cppreference_parser.py
  clang/tools/include-mapping/gen_std.py
  clang/tools/include-mapping/test.py
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/StandardLibraryTest.cpp

Index: clang/unittests/Tooling/StandardLibraryTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/StandardLibraryTest.cpp
@@ -0,0 +1,90 @@
+//===- unittest/Tooling/StandardLibrary.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Inclusions/StandardLibrary.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclarationName.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ScopedPrinter.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using ::testing::ElementsAre;
+
+namespace clang {
+namespace tooling {
+namespace {
+
+const NamedDecl &lookup(ASTUnit &AST, llvm::StringRef Name) {
+  auto &Ctx = AST.getASTContext();
+  TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+  auto Result = TU->lookup(DeclarationName(&Ctx.Idents.get(Name)));
+  assert(!Result.empty() && "Lookup failed");
+  assert(Result.isSingleResult() && "Lookup returned multiple results");
+  return *Result.front();
+}
+
+TEST(StdlibTest, All) {
+  auto VectorH = stdlib::Header::named("");
+  EXPECT_TRUE(VectorH);
+  EXPECT_EQ(llvm::to_string(*VectorH), "");
+  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) {
+  std::unique_ptr AST = buildASTFromCode(R"cpp(
+namespace std {
+inline namespace inl {
+
+template 
+struct vector { class nested {}; };
+
+class secret {};
+
+namespace chrono {}
+
+} // inl
+} // std
+
+class vector {};
+std::vector vec;
+std::vector::nested nest;
+std::secret sec;
+  )cpp");
+
+  auto &VectorNonstd = lookup(*AST, "vector");
+  auto *Vec =
+  cast(lookup(*AST, "vec")).getType()->getAsCXXRecordDecl();
+  auto *Nest =
+  cast(lookup(*AST, "nest")).getType()->getAsCXXRecordDecl();
+  auto *Sec =
+  cast(lookup(*AST, "sec")).getType()->getAsCXXRecordDecl();
+
+  stdlib::Recognizer Recognizer;
+
+  EXPECT_EQ(Recognizer(&VectorNonstd), llvm::None);
+  EXPECT_EQ(Recognizer(Vec), stdlib::Symbol::named("std::", "vector"));
+  EXPECT_EQ(Recognizer(Nest), stdlib::Symbol::named("std::", "vector"));
+  EXPECT_EQ(Recognizer(Sec), llvm::None);
+}
+
+} // namespace
+} // namespace tooling
+} // namespace clang
Index: clang/unittests/Tooling/CMakeLists.txt
===
--- clang/unittests/Tooling/CMakeLists.txt
+++ clang/unittests/Tooling/CMakeLists.txt
@@ -17,6 +17,7 @@
   ExecutionTest.cpp
   FixItTest.cpp
   HeaderIncludesTest.cpp
+  StandardLibraryTest.cpp
   LexicallyOrderedRecursiveASTVisitorTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
Index: clang-tools-extra/clangd/include-mapping/test.py
===
--- /dev/null
+++ clang-tools-extra/clangd/include-mapping/test.py
@@ -1,155 +0,0 @@
-#!/usr/bin/env python
-#===- test.py -  

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Regarding the include mapping generator, I think it would've been better if we 
had some sort of list directly from libc++ (as this is now being part of clang 
rather than just clangd), but having the current symbol mapping available for 
other tools too is definitely a useful improvement and implementation details 
can change later on.
I think we should have some "more public" documentation available around the 
limitations of current generator though, so that people are not surprised and 
aware of the caveats (like symbols might be missing, or in case of ambiguity 
they might be dropped, etc). Not sure where that belongs though, maybe header 
of the `.inc` file, or if we want it to be only used through the recognizer 
interface, maybe we can make the `inc` file "private" and document it there.

As for stdlib symbol/header/recognizer, I've got a couple questions around the 
functionality we are exposing:

- (briefly mentioned above) should we just make raw symbol mappings an 
implementation detail of stdlib recognizer and have applications depend on it?
- do we want headers/symbols to be created outside of the recognizer?




Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:15
+public:
+  static llvm::Optional named(llvm::StringRef Name);
+

can you clarify if `Name` can have quotes/angles ?



Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:37
+  static llvm::Optional named(llvm::StringRef Scope,
+  llvm::StringRef Name);
+

should scope have trailing `::` ?



Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:46
+  Header header() const;
+  // Some symbols may be provided my multiple headers.
+  llvm::SmallVector headers() const;

s/my/by



Comment at: clang/include/clang/Tooling/Inclusions/StandardLibrary.h:66
+  Recognizer();
+  llvm::Optional operator()(const Decl *D);
+

what about macros?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 406401.
kbobyrev added a comment.

Move include-mapping generators to clang and re-generate the files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

Files:
  clang-tools-extra/clangd/CSymbolMap.inc
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/StdSymbolMap.inc
  clang-tools-extra/clangd/include-mapping/cppreference_parser.py
  clang-tools-extra/clangd/include-mapping/gen_std.py
  clang-tools-extra/clangd/include-mapping/test.py
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang/include/clang/Tooling/Inclusions/CSymbolMap.inc
  clang/include/clang/Tooling/Inclusions/StandardLibrary.h
  clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc
  clang/lib/Tooling/Inclusions/CMakeLists.txt
  clang/lib/Tooling/Inclusions/StandardLibrary.cpp
  clang/tools/include-mapping/cppreference_parser.py
  clang/tools/include-mapping/gen_std.py
  clang/tools/include-mapping/test.py
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/StandardLibraryTest.cpp

Index: clang/unittests/Tooling/StandardLibraryTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/StandardLibraryTest.cpp
@@ -0,0 +1,39 @@
+//===- unittest/Tooling/StandardLibrary.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Inclusions/StandardLibrary.h"
+#include "llvm/Support/ScopedPrinter.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using ::testing::ElementsAre;
+
+namespace clang {
+namespace tooling {
+namespace {
+
+TEST(StdlibTest, All) {
+  auto VectorH = stdlib::Header::named("");
+  EXPECT_TRUE(VectorH);
+  EXPECT_EQ(llvm::to_string(*VectorH), "");
+  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));
+}
+
+} // namespace
+} // namespace tooling
+} // namespace clang
Index: clang/unittests/Tooling/CMakeLists.txt
===
--- clang/unittests/Tooling/CMakeLists.txt
+++ clang/unittests/Tooling/CMakeLists.txt
@@ -17,6 +17,7 @@
   ExecutionTest.cpp
   FixItTest.cpp
   HeaderIncludesTest.cpp
+  StandardLibraryTest.cpp
   LexicallyOrderedRecursiveASTVisitorTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
Index: clang-tools-extra/clangd/include-mapping/test.py
===
--- /dev/null
+++ clang-tools-extra/clangd/include-mapping/test.py
@@ -1,155 +0,0 @@
-#!/usr/bin/env python
-#===- test.py -  -*- python -*--===#
-#
-# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-# See https://llvm.org/LICENSE.txt for license information.
-# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-#
-#======#
-
-from cppreference_parser import _ParseSymbolPage, _ParseIndexPage
-
-import unittest
-
-class TestStdGen(unittest.TestCase):
-
-  def testParseIndexPage(self):
-html = """
- abs() (int) 
- abs<>() (std::complex) 
- acos() 
- acosh() (since C++11) 
- as_bytes<>() (since C++20) 
- """
-
-actual = _ParseIndexPage(html)
-expected = [
-  ("abs", "abs.html", True),
-  ("abs", "complex/abs.html", True),
-  ("acos", "acos.html", False),
-  ("acosh", "acosh.html", False),
-  ("as_bytes", "as_bytes.html", False),
-]
-self.assertEqual(len(actual), len(expected))
-for i in range(0, len(actual)):
-  self.assertEqual(expected[i][0], actual[i][0])
-  self.assertTrue(actual[i][1].endswith(expected[i][1]))
-  self.assertEqual(expected[i][2], actual[i][2])
-
-
-  def testParseSymbolPage_SingleHeader(self):
-# Defined in header 
-html = """
- 
-  
-   Defined in header 
-   
-  
-  
-  
-  
-void foo()
-this is matched
-  
-
-"""
-self.assertEqual(_ParseSymbolPage(html, 'foo'), set(['']))
-
-
-  def testParseSymbolPage_MulHeaders(self):
-#  Defined in header 
-#  Defined in header 
-#  Defined in header 
-html = """
-
-  
- Defined in header 
- 
- 
-
-  
-  
-void bar()
-this me

[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("");
-  EXPECT_TRUE(VectorH);
-  EXPECT_EQ(llvm::to_string(*VectorH), "");
-  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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The moved *.inc files are generated, the generator needs to be moved (and 
updated?) too I think.

Is having clangd continue to include the .inc files textually intended to stay 
that way, or is it a FIXME?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits