[PATCH] D59639: [clangd] Print template arguments helper

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.



Comment at: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt:16
   Annotations.cpp
+  PrintASTTests.cpp
   BackgroundIndexTests.cpp

Keep alphabetized?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59639



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

Re: [clangd] Print template arguments helper

[Kadir Çetinkaya via cfe-commits]

Thanks Bruno, sent out rL358293 to address the issue.

On Fri, Apr 12, 2019 at 5:21 PM Bruno Ricci  wrote:

> Hi,
>
> It seems that one of r358272, r358273 or r358274 is causing some asan
> failure on my machine. Not sure why it is not spotted by the bots.
>
> Failure log attached.
>
> Bruno
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clangd] Print template arguments helper

[Bruno Ricci via cfe-commits]

Hi,

It seems that one of r358272, r358273 or r358274 is causing some asan
failure on my machine. Not sure why it is not spotted by the bots.

Failure log attached.

Bruno
FAIL: Extra Tools Unit Tests :: 
clangd/./ClangdTests/ASTUtilsTests/ASTUtils.PrintTemplateArgs/1 (878 of 1313)
 TEST 'Extra Tools Unit Tests :: 
clangd/./ClangdTests/ASTUtilsTests/ASTUtils.PrintTemplateArgs/1' FAILED 

Note: Google Test filter = ASTUtilsTests/ASTUtils.PrintTemplateArgs/1
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from ASTUtilsTests/ASTUtils
[ RUN  ] ASTUtilsTests/ASTUtils.PrintTemplateArgs/1
Preamble for file /clangd-test/TestTU.cpp cannot be reused. Attempting to 
rebuild it.
Built preamble of size 199084 for file /clangd-test/TestTU.cpp
=
==14611==ERROR: AddressSanitizer: heap-buffer-overflow on address 
0x602075a0 at pc 0x020df0e5 bp 0x7ffd01321e30 sp 0x7ffd01321e28
READ of size 4 at 0x602075a0 thread T0
#0 0x20df0e4 in __eq 
/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/tuple:1388:36
#1 0x20df0e4 in operator== /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/tuple:1421
#2 0x20df0e4 in operator== 
/home/bruno/software/llvm/tools/clang/tools/extra/clangd/Protocol.h:136
#3 0x20df0e4 in operator!= 
/home/bruno/software/llvm/tools/clang/tools/extra/clangd/Protocol.h:140
#4 0x20df0e4 in VisitNamedDecl 
/home/bruno/software/llvm/tools/clang/tools/extra/unittests/clangd/PrintASTTests.cpp:42
#5 0x20df0e4 in clang::RecursiveASTVisitor::WalkUpFromNamedDecl(clang::NamedDecl*)
 /mnt/data/llvm-asan-build/tools/clang/include/clang/AST/DeclNodes.inc:95
#6 0x20a7307 in WalkUpFromTypeDecl 
/mnt/data/llvm-asan-build/tools/clang/include/clang/AST/DeclNodes.inc:233:1
#7 0x20a7307 in WalkUpFromTemplateTypeParmDecl 
/mnt/data/llvm-asan-build/tools/clang/include/clang/AST/DeclNodes.inc:281
#8 0x20a7307 in TraverseTemplateTypeParmDecl 
/home/bruno/software/llvm/tools/clang/include/clang/AST/RecursiveASTVisitor.h:1770
#9 0x20a7307 in clang::RecursiveASTVisitor::TraverseDecl(clang::Decl*)
 /mnt/data/llvm-asan-build/tools/clang/include/clang/AST/DeclNodes.inc:281
#10 0x20a7685 in TraverseClassTemplatePartialSpecializationDecl 
/home/bruno/software/llvm/tools/clang/include/clang/AST/RecursiveASTVisitor.h:1910:1
#11 0x20a7685 in clang::RecursiveASTVisitor::TraverseDecl(clang::Decl*)
 /mnt/data/llvm-asan-build/tools/clang/include/clang/AST/DeclNodes.inc:259
#12 0x20ad76a in clang::RecursiveASTVisitor::TraverseDeclContextHelper(clang::DeclContext*)
 
/home/bruno/software/llvm/tools/clang/include/clang/AST/RecursiveASTVisitor.h:1387:7
#13 0x20a7116 in clang::RecursiveASTVisitor::TraverseDecl(clang::Decl*)
 /home/bruno/software/llvm/tools/clang/include/clang/AST/Decl.h
#14 0x20a5cf9 in clang::clangd::(anonymous 
namespace)::ASTUtils_PrintTemplateArgs_Test::TestBody() 
/home/bruno/software/llvm/tools/clang/tools/extra/unittests/clangd/PrintASTTests.cpp:51:5
#15 0x2d17dd8 in testing::Test::Run() 
/home/bruno/software/llvm/utils/unittest/googletest/src/gtest.cc
#16 0x2d1ca4a in testing::TestInfo::Run() 
/home/bruno/software/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
#17 0x2d1e280 in testing::TestCase::Run() 
/home/bruno/software/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
#18 0x2d37d9d in testing::internal::UnitTestImpl::RunAllTests() 
/home/bruno/software/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
#19 0x2d365b6 in testing::UnitTest::Run() 
/home/bruno/software/llvm/utils/unittest/googletest/src/gtest.cc
#20 0x2cfef66 in RUN_ALL_TESTS 
/home/bruno/software/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
#21 0x2cfef66 in main 
/home/bruno/software/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50
#22 0x7f04a1a8a09a in __libc_start_main 
/build/glibc-B9XfQf/glibc-2.28/csu/../csu/libc-start.c:308:16
#23 0x1f97029 in _start 
(/mnt/data/llvm-asan-build/tools/clang/tools/extra/unittests/clangd/ClangdTests+0x1f97029)

0x602075a0 is located 0 bytes to the right of 16-byte region 
[0x60207590,0x602075a0)
allocated by thread T0 here:
#0 0x20984c0 in operator new(unsigned long) 
(/mnt/data/llvm-asan-build/tools/clang/tools/extra/unittests/clangd/ClangdTests+0x20984c0)
#1 0x209ec66 in allocate 
/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ext/new_allocator.h:111:27
#2 0x209ec66 in allocate 
/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/alloc_traits.h:436
#3 0x209ec66 in _M_allocate 
/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/stl_vector.h:296
#4 0x209ec66 in _M_range_initialize 
/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/stl_vector.h:1466
#5 0x209ec66 in _M_initialize_dispatch 

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358272: [clangd] Print template arguments helper (authored 
by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59639?vs=194686=194830#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59639

Files:
  cfe/trunk/lib/AST/TypePrinter.cpp
  clang-tools-extra/trunk/clangd/AST.cpp
  clang-tools-extra/trunk/clangd/AST.h
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
  clang-tools-extra/trunk/unittests/clangd/PrintASTTests.cpp

Index: cfe/trunk/lib/AST/TypePrinter.cpp
===
--- cfe/trunk/lib/AST/TypePrinter.cpp
+++ cfe/trunk/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,19 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const TemplateArgument::ArgKind  = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1666,8 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  // Tries to print the argument with location info if exists.
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
===
--- clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
+++ clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
@@ -13,6 +13,7 @@
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  PrintASTTests.cpp
   BackgroundIndexTests.cpp
   CancellationTests.cpp
   ClangdTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/PrintASTTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/PrintASTTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/PrintASTTests.cpp
@@ -0,0 +1,100 @@
+//===--- PrintASTTests.cpp - C++-*-===//
+//
+// 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 "AST.h"
+#include "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-param-test.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-param-util-generated.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAreArray;
+
+struct Case {
+  const char *AnnotatedCode;
+  std::vector Expected;
+};
+class ASTUtils : public testing::Test,
+ public ::testing::WithParamInterface {};
+
+TEST_P(ASTUtils, PrintTemplateArgs) {
+  auto Pair = GetParam();
+  Annotations Test(Pair.AnnotatedCode);
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor {
+Visitor(std::vector Points) : Points(std::move(Points)) {}
+bool VisitNamedDecl(const NamedDecl *ND) {
+  auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+ ND->getLocation());
+  if (Pos != Points[TemplateArgsAtPoints.size()])
+return true;
+  TemplateArgsAtPoints.push_back(printTemplateSpecializationArgs(*ND));
+  return true;
+}
+std::vector TemplateArgsAtPoints;
+const std::vector Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgsAtPoints, ElementsAreArray(Pair.Expected));
+}
+
+INSTANTIATE_TEST_CASE_P(ASTUtilsTests, ASTUtils,
+testing::ValuesIn(std::vector({
+{
+R"cpp(
+  template  class Bar {};
+  template <> class ^Bar {};)cpp",
+{""}},
+{
+R"cpp(
+  

r358272 - [clangd] Print template arguments helper

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Fri Apr 12 03:09:14 2019
New Revision: 358272

URL: http://llvm.org/viewvc/llvm-project?rev=358272=rev
Log:
[clangd] Print template arguments helper

Summary:
Prepares ground for printing template arguments as written in the
source code, part of re-landing rC356541 with D59599 applied.

Reviewers: ioeric, ilya-biryukov

Subscribers: mgorny, MaskRay, jkorous, arphaman, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59639

Modified:
cfe/trunk/lib/AST/TypePrinter.cpp

Modified: cfe/trunk/lib/AST/TypePrinter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/TypePrinter.cpp?rev=358272=358271=358272=diff
==
--- cfe/trunk/lib/AST/TypePrinter.cpp (original)
+++ cfe/trunk/lib/AST/TypePrinter.cpp Fri Apr 12 03:09:14 2019
@@ -1632,6 +1632,19 @@ static const TemplateArgument 
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const TemplateArgument::ArgKind  = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1666,8 @@ static void printTo(raw_ostream , Arr
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  // Tries to print the argument with location info if exists.
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 


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

[clang-tools-extra] r358272 - [clangd] Print template arguments helper

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Fri Apr 12 03:09:14 2019
New Revision: 358272

URL: http://llvm.org/viewvc/llvm-project?rev=358272=rev
Log:
[clangd] Print template arguments helper

Summary:
Prepares ground for printing template arguments as written in the
source code, part of re-landing rC356541 with D59599 applied.

Reviewers: ioeric, ilya-biryukov

Subscribers: mgorny, MaskRay, jkorous, arphaman, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59639

Added:
clang-tools-extra/trunk/unittests/clangd/PrintASTTests.cpp
Modified:
clang-tools-extra/trunk/clangd/AST.cpp
clang-tools-extra/trunk/clangd/AST.h
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp

Modified: clang-tools-extra/trunk/clangd/AST.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/AST.cpp?rev=358272=358271=358272=diff
==
--- clang-tools-extra/trunk/clangd/AST.cpp (original)
+++ clang-tools-extra/trunk/clangd/AST.cpp Fri Apr 12 03:09:14 2019
@@ -11,15 +11,37 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/TemplateBase.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 
+namespace {
+llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl ) {
+  if (auto *Func = llvm::dyn_cast()) {
+if (const ASTTemplateArgumentListInfo *Args =
+Func->getTemplateSpecializationArgsAsWritten())
+  return Args->arguments();
+  } else if (auto *Cls =
+ llvm::dyn_cast()) {
+if (auto *Args = Cls->getTemplateArgsAsWritten())
+  return Args->arguments();
+  } else if (auto *Var = llvm::dyn_cast())
+return Var->getTemplateArgsInfo().arguments();
+  // We return None for ClassTemplateSpecializationDecls because it does not
+  // contain TemplateArgumentLoc information.
+  return llvm::None;
+}
+} // namespace
+
 // Returns true if the complete name of decl \p D is spelled in the source 
code.
 // This is not the case for:
 //   * symbols formed via macro concatenation, the spelling location will
@@ -65,17 +87,6 @@ std::string printQualifiedName(const Nam
   return QName;
 }
 
-static const TemplateArgumentList *
-getTemplateSpecializationArgs(const NamedDecl ) {
-  if (auto *Func = llvm::dyn_cast())
-return Func->getTemplateSpecializationArgs();
-  if (auto *Cls = llvm::dyn_cast())
-return >getTemplateInstantiationArgs();
-  if (auto *Var = llvm::dyn_cast())
-return >getTemplateInstantiationArgs();
-  return nullptr;
-}
-
 std::string printName(const ASTContext , const NamedDecl ) {
   std::string Name;
   llvm::raw_string_ostream Out(Name);
@@ -90,9 +101,7 @@ std::string printName(const ASTContext &
   }
   ND.getDeclName().print(Out, PP);
   if (!Out.str().empty()) {
-// FIXME(ibiryukov): do not show args not explicitly written by the user.
-if (auto *ArgList = getTemplateSpecializationArgs(ND))
-  printTemplateArgumentList(Out, ArgList->asArray(), PP);
+Out << printTemplateSpecializationArgs(ND);
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
@@ -105,6 +114,35 @@ std::string printName(const ASTContext &
   return "(anonymous)";
 }
 
+std::string printTemplateSpecializationArgs(const NamedDecl ) {
+  std::string TemplateArgs;
+  llvm::raw_string_ostream OS(TemplateArgs);
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  if (llvm::Optional> Args =
+  getTemplateSpecializationArgLocs(ND)) {
+printTemplateArgumentList(OS, *Args, Policy);
+  } else if (auto *Cls = llvm::dyn_cast()) 
{
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {
+  // ClassTemplateSpecializationDecls do not contain
+  // TemplateArgumentTypeLocs, they only have TemplateArgumentTypes. So we
+  // create a new argument location list from TypeSourceInfo.
+  auto STL = TSI->getTypeLoc().getAs();
+  llvm::SmallVector ArgLocs;
+  ArgLocs.reserve(STL.getNumArgs());
+  for (unsigned I = 0; I < STL.getNumArgs(); ++I)
+ArgLocs.push_back(STL.getArgLoc(I));
+  printTemplateArgumentList(OS, ArgLocs, Policy);
+} else {
+  // FIXME: Fix cases when getTypeAsWritten returns null inside clang AST,
+  // e.g. friend decls. Currently we fallback to Template Arguments without
+  // location information.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Poli

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:139
+  // location information.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
+}

ioeric wrote:
> Could you also add a test case for this with the current behavior and a FIXME?
Adding the test case, but the problem is there wouldn't be a change in 
behavior(at least not in the cases that I can think of) since a full 
specialization doesn't have any dependent types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 194686.
kadircet marked 6 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp
  clang-tools-extra/unittests/clangd/PrintASTTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,19 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const TemplateArgument::ArgKind  = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1666,8 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  // Tries to print the argument with location info if exists.
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/PrintASTTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/PrintASTTests.cpp
@@ -0,0 +1,100 @@
+//===--- PrintASTTests.cpp - C++-*-===//
+//
+// 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 "AST.h"
+#include "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-param-test.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-param-util-generated.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAreArray;
+
+struct Case {
+  const char *AnnotatedCode;
+  std::vector Expected;
+};
+class ASTUtils : public testing::Test,
+ public ::testing::WithParamInterface {};
+
+TEST_P(ASTUtils, PrintTemplateArgs) {
+  auto Pair = GetParam();
+  Annotations Test(Pair.AnnotatedCode);
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor {
+Visitor(std::vector Points) : Points(std::move(Points)) {}
+bool VisitNamedDecl(const NamedDecl *ND) {
+  auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+ ND->getLocation());
+  if (Pos != Points[TemplateArgsAtPoints.size()])
+return true;
+  TemplateArgsAtPoints.push_back(printTemplateSpecializationArgs(*ND));
+  return true;
+}
+std::vector TemplateArgsAtPoints;
+const std::vector Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgsAtPoints, ElementsAreArray(Pair.Expected));
+}
+
+INSTANTIATE_TEST_CASE_P(ASTUtilsTests, ASTUtils,
+testing::ValuesIn(std::vector({
+{
+R"cpp(
+  template  class Bar {};
+  template <> class ^Bar {};)cpp",
+{""}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class Z, int Q>
+  struct Foo {};
+  template struct ^Foo;
+  template 
+  struct ^Foo {};)cpp",
+{"", ""}},
+{
+R"cpp(
+  template  void Foz() {};
+  template <> void ^Foz<3, 5, 8>() {};)cpp",
+{"<3, 5, 8>"}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class ...>
+  class Aux {};

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: clang-tools-extra/clangd/AST.cpp:139
+  // location information.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
+}

Could you also add a test case for this with the current behavior and a FIXME?



Comment at: clang-tools-extra/unittests/clangd/PrintASTTests.cpp:47
+}
+std::vector TemplateArgs;
+const std::vector Points;

maybe `s/TemplateArgs/TemplateArgsAtPoints/` for clarity


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 194658.
kadircet added a comment.

- Update file comment for PrintASTTests.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp
  clang-tools-extra/unittests/clangd/PrintASTTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,19 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const TemplateArgument::ArgKind  = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1666,8 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  // Tries to print the argument with location info if exists.
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/PrintASTTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/PrintASTTests.cpp
@@ -0,0 +1,94 @@
+//===--- PrintASTTests.cpp - C++-*-===//
+//
+// 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 "AST.h"
+#include "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-param-test.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-param-util-generated.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAreArray;
+
+struct Case {
+  const char *AnnotatedCode;
+  std::vector Expected;
+};
+class ASTUtils : public testing::Test,
+ public ::testing::WithParamInterface {};
+
+TEST_P(ASTUtils, PrintTemplateArgs) {
+  auto Pair = GetParam();
+  Annotations Test(Pair.AnnotatedCode);
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor {
+Visitor(std::vector Points) : Points(std::move(Points)) {}
+bool VisitNamedDecl(const NamedDecl *ND) {
+  auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+ ND->getLocation());
+  if (Pos != Points[TemplateArgs.size()])
+return true;
+  TemplateArgs.push_back(printTemplateSpecializationArgs(*ND));
+  return true;
+}
+std::vector TemplateArgs;
+const std::vector Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgs, ElementsAreArray(Pair.Expected));
+}
+
+INSTANTIATE_TEST_CASE_P(ASTUtilsTests, ASTUtils,
+testing::ValuesIn(std::vector({
+{
+R"cpp(
+  template  class Bar {};
+  template <> class ^Bar {};)cpp",
+{""}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class Z, int Q>
+  struct Foo {};
+  template struct ^Foo;
+  template 
+  struct ^Foo {};)cpp",
+{"", ""}},
+{
+R"cpp(
+  template  void Foz() {};
+  template <> void ^Foz<3, 5, 8>() {};)cpp",
+{"<3, 5, 8>"}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class ...>
+  class Aux {};
+  template <> 

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:133
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {

ioeric wrote:
> kadircet wrote:
> > ioeric wrote:
> > > why isn't this handled in `getTemplateSpecializationArgLocs`? Add a 
> > > comment?
> > it is mentioned in `getTemplateSpecializationArgLocs`, would you rather 
> > move the comment here?
> I think it would be clearer if we do something like:
> 
> ```
> if (auto *Cls = llvm::dyn_cast()) {
>   // handle this specially because ...
> } else {
>   // use getTemplateSpecializationArgLocs to handle rest of cases.
> }
> 
> ```
I am not performing this in the order you mentioned since 
`ClassTemplatePartialSpecializationDecl` is also a 
`ClassTemplateSpecializationDecl`, but if you insist I can change the ordering 
by adding another condition(which just looks ugly).



Comment at: clang-tools-extra/clangd/AST.cpp:149
+} else {
+  // FIXME: Fix cases when getTypeAsWritten returns null, e.g. friend 
decls.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);

ioeric wrote:
> I'm not quite sure if I understand this FIXME. In this `else` branch, we are 
> handling this case. Do you mean this is not a proper fix and future work is 
> needed here? Could you elaborate?
> 
> One thing that's worth commenting is why we could use 
> `Cls->getTemplateArgs().asArray()` when `Cls->getTypeAsWritten` is null. It's 
> not trivial from the code.
Yes, this should rather be fixed in AST itself. Updating comment to explain the 
fallback strategy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 194657.
kadircet marked 7 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/FindSymbolsTests.cpp
  clang-tools-extra/unittests/clangd/PrintASTTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,19 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const TemplateArgument::ArgKind  = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1666,8 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  // Tries to print the argument with location info if exists.
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/PrintASTTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/PrintASTTests.cpp
@@ -0,0 +1,94 @@
+//===--- ASTUtilsTests.cpp - C++-*-===//
+//
+// 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 "AST.h"
+#include "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-param-test.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-param-util-generated.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAreArray;
+
+struct Case {
+  const char *AnnotatedCode;
+  std::vector Expected;
+};
+class ASTUtils : public testing::Test,
+ public ::testing::WithParamInterface {};
+
+TEST_P(ASTUtils, PrintTemplateArgs) {
+  auto Pair = GetParam();
+  Annotations Test(Pair.AnnotatedCode);
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor {
+Visitor(std::vector Points) : Points(std::move(Points)) {}
+bool VisitNamedDecl(const NamedDecl *ND) {
+  auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+ ND->getLocation());
+  if (Pos != Points[TemplateArgs.size()])
+return true;
+  TemplateArgs.push_back(printTemplateSpecializationArgs(*ND));
+  return true;
+}
+std::vector TemplateArgs;
+const std::vector Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgs, ElementsAreArray(Pair.Expected));
+}
+
+INSTANTIATE_TEST_CASE_P(ASTUtilsTests, ASTUtils,
+testing::ValuesIn(std::vector({
+{
+R"cpp(
+  template  class Bar {};
+  template <> class ^Bar {};)cpp",
+{""}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class Z, int Q>
+  struct Foo {};
+  template struct ^Foo;
+  template 
+  struct ^Foo {};)cpp",
+{"", ""}},
+{
+R"cpp(
+  template  void Foz() {};
+  template <> void ^Foz<3, 5, 8>() {};)cpp",
+{"<3, 5, 8>"}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class ...>
+  class Aux {};
+

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:149
+} else {
+  // FIXME: Fix cases when getTypeAsWritten returns null, e.g. friend 
decls.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);

I'm not quite sure if I understand this FIXME. In this `else` branch, we are 
handling this case. Do you mean this is not a proper fix and future work is 
needed here? Could you elaborate?

One thing that's worth commenting is why we could use 
`Cls->getTemplateArgs().asArray()` when `Cls->getTypeAsWritten` is null. It's 
not trivial from the code.



Comment at: clang-tools-extra/clangd/AST.cpp:88
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl ) {
   if (auto *Func = llvm::dyn_cast())

kadircet wrote:
> ioeric wrote:
> > can we unify this with `getTemplateSpecializationArgLocs` somehow? 
> > 
> > it seems that args as written would also be favorable here (see FIXME in 
> > line 112)
> See D59641
thought?



Comment at: clang-tools-extra/clangd/AST.cpp:133
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {

kadircet wrote:
> ioeric wrote:
> > why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?
> it is mentioned in `getTemplateSpecializationArgLocs`, would you rather move 
> the comment here?
I think it would be clearer if we do something like:

```
if (auto *Cls = llvm::dyn_cast()) {
  // handle this specially because ...
} else {
  // use getTemplateSpecializationArgLocs to handle rest of cases.
}

```



Comment at: clang/lib/AST/TypePrinter.cpp:1635
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {

kadircet wrote:
> ioeric wrote:
> > could you add clang tests for these changes?
> We've also discussed this in the original comment and decided infrastructure 
> necessary was too overwhelming. First we need to invoke compiler and get the 
> AST, then write a Visitor to fetch relevant decls and only after that call 
> printTemplateArguments ...
No test at all is concerning... I think there are lit tests for AST printing in 
test/AST/. Is it possible to have some of those?



Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {

kadircet wrote:
> ioeric wrote:
> > It's unclear to me what the new behavior is with changes in this file. 
> > Could you add comment?
> > 
> > It might make sense to split the clang change into a separate patch and get 
> > folks who are more familiar to take a look.
> it just prevents erasure from TypeLoc to Type when printing the argument, so 
> that we can do a fine-grained printing if Location is already there.
could you add this into comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:26
+llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl ) {
+  if (auto *Func = llvm::dyn_cast()) {

ilya-biryukov wrote:
> Eugene.Zelenko wrote:
> > Functions should be static, not in anonymous namespace. See LLVM Coding 
> > Guidelines.
> We tend to put internal functions to anonymous namespace quite a lot in 
> clangd.
> While technically a small violation of the LLVM style guide, this aligns with 
> the rest of the code and we don't consider that to be a problem.
I don't think it's reasonable to have one style per project. Even if clangd has 
such historical code, it'll be good idea to change it to confirm to common 
guidelines. Evolution of LLDB formatting style is good example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 191857.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/PrintASTTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,19 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const TemplateArgument::ArgKind  = A.getArgument().getKind();
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1666,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/PrintASTTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/PrintASTTests.cpp
@@ -0,0 +1,94 @@
+//===--- ASTUtilsTests.cpp - C++-*-===//
+//
+// 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 "AST.h"
+#include "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest-param-test.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-param-util-generated.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAreArray;
+
+struct Case {
+  const char *AnnotatedCode;
+  std::vector Expected;
+};
+class ASTUtils : public testing::Test,
+ public ::testing::WithParamInterface {};
+
+TEST_P(ASTUtils, PrintTemplateArgs) {
+  auto Pair = GetParam();
+  Annotations Test(Pair.AnnotatedCode);
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor {
+Visitor(std::vector Points) : Points(std::move(Points)) {}
+bool VisitNamedDecl(const NamedDecl *ND) {
+  auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+ ND->getLocation());
+  if (Pos != Points[TemplateArgs.size()])
+return true;
+  TemplateArgs.push_back(printTemplateSpecializationArgs(*ND));
+  return true;
+}
+std::vector TemplateArgs;
+const std::vector Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgs, ElementsAreArray(Pair.Expected));
+}
+
+INSTANTIATE_TEST_CASE_P(ASTUtilsTests, ASTUtils,
+testing::ValuesIn(std::vector({
+{
+R"cpp(
+  template  class Bar {};
+  template <> class ^Bar {};)cpp",
+{""}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class Z, int Q>
+  struct Foo {};
+  template struct ^Foo;
+  template 
+  struct ^Foo {};)cpp",
+{"", ""}},
+{
+R"cpp(
+  template  void Foz() {};
+  template <> void ^Foz<3, 5, 8>() {};)cpp",
+{"<3, 5, 8>"}},
+{
+R"cpp(
+  template  class Bar {};
+  template  class ...>
+  class Aux {};
+  template <> class ^Aux {};
+  template  T>
+  class ^Aux 

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 15 inline comments as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:88
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl ) {
   if (auto *Func = llvm::dyn_cast())

ioeric wrote:
> can we unify this with `getTemplateSpecializationArgLocs` somehow? 
> 
> it seems that args as written would also be favorable here (see FIXME in line 
> 112)
See D59641



Comment at: clang-tools-extra/clangd/AST.cpp:133
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {

ioeric wrote:
> why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?
it is mentioned in `getTemplateSpecializationArgLocs`, would you rather move 
the comment here?



Comment at: clang/lib/AST/TypePrinter.cpp:1635
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {

ioeric wrote:
> could you add clang tests for these changes?
We've also discussed this in the original comment and decided infrastructure 
necessary was too overwhelming. First we need to invoke compiler and get the 
AST, then write a Visitor to fetch relevant decls and only after that call 
printTemplateArguments ...



Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {

ioeric wrote:
> It's unclear to me what the new behavior is with changes in this file. Could 
> you add comment?
> 
> It might make sense to split the clang change into a separate patch and get 
> folks who are more familiar to take a look.
it just prevents erasure from TypeLoc to Type when printing the argument, so 
that we can do a fine-grained printing if Location is already there.



Comment at: clang/lib/AST/TypePrinter.cpp:1643
+  const auto  = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+ "TemplateArgumentKind can not be null!");

ioeric wrote:
> why? 
> 
> ```
> /// Represents an empty template argument, e.g., one that has not
> /// been deduced.
> ```
> It seems legitimate. 
> 
> Since this hot code path, we would want to avoid hard assertion if possible.
Sure, letting TemplateArgument::print handle that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:26
+llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl ) {
+  if (auto *Func = llvm::dyn_cast()) {

Eugene.Zelenko wrote:
> Functions should be static, not in anonymous namespace. See LLVM Coding 
> Guidelines.
We tend to put internal functions to anonymous namespace quite a lot in clangd.
While technically a small violation of the LLVM style guide, this aligns with 
the rest of the code and we don't consider that to be a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp:1
+#include "AST.h"
+#include "Annotations.h"

NIT: add a licence header



Comment at: clang-tools-extra/unittests/clangd/CMakeLists.txt:13
   Annotations.cpp
+  ASTUtilsTests.cpp
   BackgroundIndexTests.cpp

NIT: maybe call it `PrintASTTests`?
To avoid creating a dump of test for all the stuff we are too lazy to classify 
properly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {

It's unclear to me what the new behavior is with changes in this file. Could 
you add comment?

It might make sense to split the clang change into a separate patch and get 
folks who are more familiar to take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:88
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl ) {
   if (auto *Func = llvm::dyn_cast())

can we unify this with `getTemplateSpecializationArgLocs` somehow? 

it seems that args as written would also be favorable here (see FIXME in line 
112)



Comment at: clang-tools-extra/clangd/AST.cpp:131
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+printTemplateArgumentList(OS, *Args, Policy);

Eugene.Zelenko wrote:
> Return type is not obvious, so auto should not be used.
nit: I'd suggest keeping `{}` for symmetry.



Comment at: clang-tools-extra/clangd/AST.cpp:133
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {

why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?



Comment at: clang-tools-extra/clangd/AST.cpp:134
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {
+  auto STL = TSI->getTypeLoc().getAs();

could you add comment/example explaining what's special about this case?



Comment at: clang-tools-extra/clangd/AST.h:54
+/// Returns an empty string if type is not a template specialization.
+std::string printTemplateArgsAsWritten(const NamedDecl );
+

Maybe `printTemplateSpecializationArgs` since we are only handling template 
specialization? 

I think we could drop `AsWritten` from the name. It should be clear to just 
explain in the comment.  



Comment at: clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp:16
+
+TEST(ASTUtils, PrintTemplateArgs) {
+  Annotations Test(R"cpp(

I think this kind of tests would be more readable using `TEST_P` [1] with code 
and expected arguments as parameters.

Up to you though.

[1] 
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters



Comment at: clang/lib/AST/TypePrinter.cpp:1635
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {

could you add clang tests for these changes?



Comment at: clang/lib/AST/TypePrinter.cpp:1643
+  const auto  = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+ "TemplateArgumentKind can not be null!");

why? 

```
/// Represents an empty template argument, e.g., one that has not
/// been deduced.
```
It seems legitimate. 

Since this hot code path, we would want to avoid hard assertion if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:26
+llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl ) {
+  if (auto *Func = llvm::dyn_cast()) {

Functions should be static, not in anonymous namespace. See LLVM Coding 
Guidelines.



Comment at: clang-tools-extra/clangd/AST.cpp:28
+  if (auto *Func = llvm::dyn_cast()) {
+if (auto *Args = Func->getTemplateSpecializationArgsAsWritten())
+  return Args->arguments();

Return type is not obvious, so auto should not be used.



Comment at: clang-tools-extra/clangd/AST.cpp:131
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+printTemplateArgumentList(OS, *Args, Policy);

Return type is not obvious, so auto should not be used.



Comment at: clang/lib/AST/TypePrinter.cpp:1642
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const auto  = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&

Return type is not obvious, so auto should not be used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: ioeric, ilya-biryukov.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, mgorny.
Herald added a project: clang.

Prepares ground for printing template arguments as written in the
source code, part of re-landing rC356541  
with D59599  applied.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59639

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,21 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {
+  const auto  = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+ "TemplateArgumentKind can not be null!");
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream , ArrayRef Args,
 const PrintingPolicy , bool SkipBrackets) {
@@ -1653,7 +1668,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/CMakeLists.txt
===
--- clang-tools-extra/unittests/clangd/CMakeLists.txt
+++ clang-tools-extra/unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
 
 add_extra_unittest(ClangdTests
   Annotations.cpp
+  ASTUtilsTests.cpp
   BackgroundIndexTests.cpp
   CancellationTests.cpp
   ClangdTests.cpp
Index: clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp
@@ -0,0 +1,63 @@
+#include "AST.h"
+#include "Annotations.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::ElementsAre;
+
+TEST(ASTUtils, PrintTemplateArgs) {
+  Annotations Test(R"cpp(
+  template  class Bar {};
+  template <> class ^Bar {};
+
+  template  class Z, int Q> struct Foo {};
+  template struct ^Foo;
+  template  struct ^Foo {};
+
+  template  class Baz {};
+  template <> class ^Baz {};
+  template  class ^Baz {};
+
+  template  void Foz() {};
+  template <> void ^Foz<3, 5, 8>() {};
+
+  template  class ...> class Aux {};
+  template <> class ^Aux {};
+  template  T> class ^Aux {};
+
+  template  T var = 1234;
+  template <> int ^var = 1;
+  )cpp");
+  auto AST = TestTU::withCode(Test.code()).build();
+  struct Visitor : RecursiveASTVisitor {
+Visitor(std::vector Points) : Points(std::move(Points)) {}
+bool VisitNamedDecl(const NamedDecl *ND) {
+  auto Pos = sourceLocToPosition(ND->getASTContext().getSourceManager(),
+ ND->getLocation());
+  if (Pos != Points[TemplateArgs.size()])
+return true;
+  TemplateArgs.push_back(printTemplateArgsAsWritten(*ND));
+  return true;
+}
+std::vector TemplateArgs;
+const std::vector Points;
+  };
+  Visitor V(Test.points());
+  V.TraverseDecl(AST.getASTContext().getTranslationUnitDecl());
+  EXPECT_THAT(V.TemplateArgs,
+  ElementsAre("", "", "",
+  "", "", "<3, 5, 8>", "",
+  "", ""));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/AST.h
===
--- clang-tools-extra/clangd/AST.h
+++ clang-tools-extra/clangd/AST.h
@@ -47,6 +47,12 @@
 /// "(anonymous struct)" or "(anonymous namespace)".
 std::string printName(const ASTContext , const NamedDecl );
 
+/// Prints template arguments of a decl including enclosing '<' and '>', e.g for
+/// a partial specialization like: template  struct Foo will
+/// return ''.
+/// Returns an empty string if type is not a template specialization.
+std::string printTemplateArgsAsWritten(const NamedDecl );
+
 /// Gets the symbol ID for a declaration, if possible.
 llvm::Optional getSymbolID(const Decl *D);
 
Index: clang-tools-extra/clangd/AST.cpp
===
---