[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

Tests are happy again after 1c66d09 
 , thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

Re: [PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread Kadir Çetinkaya via cfe-commits 
done. sorry for forgetting about this so soon.

On Thu, Oct 31, 2019 at 12:58 PM Nico Weber via Phabricator <
revi...@reviews.llvm.org> wrote:

> thakis added inline comments.
>
>
> 
> Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1388
>// results in arbitrary failures as function body becomes NULL.
>ExtraArgs.push_back("-fno-delayed-template-parsing");
> +  for (const auto  : Cases)
> 
> You might need something like this in some of the new tests.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D68937/new/
>
> https://reviews.llvm.org/D68937
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

This is failing on Windows: http://45.33.8.238/win/1481/step_7.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread Nico Weber via Phabricator via cfe-commits 
thakis added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1388
   // results in arbitrary failures as function body becomes NULL.
   ExtraArgs.push_back("-fno-delayed-template-parsing");
+  for (const auto  : Cases)

You might need something like this in some of the new tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG71aa3f7b7e43: [clangd] Add parameter renaming to 
define-inline code action (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -40,9 +40,9 @@
 #include 
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
-using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
@@ -1386,7 +1386,7 @@
   // Template body is not parsed until instantiation time on windows, which
   // results in arbitrary failures as function body becomes NULL.
   ExtraArgs.push_back("-fno-delayed-template-parsing");
-  for(const auto  : Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1429,7 +1429,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDeclRefs) {
-  auto Test =R"cpp(
+  auto Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -1498,6 +1498,70 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  std::pair Cases[] = {
+  {R"cpp(
+void foo(int, bool b, int T\
+est);
+void ^foo(int f, bool x, int z) {})cpp",
+   R"cpp(
+void foo(int f, bool x, int z){}
+)cpp"},
+  {R"cpp(
+#define PARAM int Z
+void foo(PARAM);
+
+void ^foo(int X) {})cpp",
+   "fail: Cant rename parameter inside macro body."},
+  {R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+template 
+void ^foo(int Z, int b, int c, int d) {})cpp",
+   R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+)cpp"},
+  };
+  for (const auto  : Cases)
+EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  auto Test = R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp";
+  auto Expected = R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp";
+  EXPECT_EQ(apply(Test), Expected);
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
@@ -1536,7 +1600,7 @@
   void fo^o() { return ; })cpp",
"fail: Couldn't find semicolon for target declaration."},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1594,7 +1658,7 @@
   void TARGET(){ return; }
   )cpp"},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,107 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap ParamToNewName;
+  llvm::DenseMap> RefLocs;
+  auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+// No need to rename if parameters already have the same name.
+if (DestParam->getName() == SourceParam->getName())
+  return;
+std::string NewName;
+// Unnamed parameters won't be visited in findExplicitReferences. So add
+// them here.
+if (DestParam->getName().empty()) {
+  RefLocs[DestParam].push_back(DestParam->getLocation());
+  // If decl is unnamed in destination we pad the new name to avoid gluing
+  // with previous token, e.g. foo(int^) shouldn't turn

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-31 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 227233.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments
- Get rid of raw string literals inside macro calls, to not break stage1 
compilers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -40,9 +40,9 @@
 #include 
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
-using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
@@ -1377,7 +1377,7 @@
   // Template body is not parsed until instantiation time on windows, which
   // results in arbitrary failures as function body becomes NULL.
   ExtraArgs.push_back("-fno-delayed-template-parsing");
-  for(const auto  : Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1420,7 +1420,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDeclRefs) {
-  auto Test =R"cpp(
+  auto Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -1489,6 +1489,70 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  std::pair Cases[] = {
+  {R"cpp(
+void foo(int, bool b, int T\
+est);
+void ^foo(int f, bool x, int z) {})cpp",
+   R"cpp(
+void foo(int f, bool x, int z){}
+)cpp"},
+  {R"cpp(
+#define PARAM int Z
+void foo(PARAM);
+
+void ^foo(int X) {})cpp",
+   "fail: Cant rename parameter inside macro body."},
+  {R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+template 
+void ^foo(int Z, int b, int c, int d) {})cpp",
+   R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+)cpp"},
+  };
+  for (const auto  : Cases)
+EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  auto Test = R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp";
+  auto Expected = R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp";
+  EXPECT_EQ(apply(Test), Expected);
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
@@ -1527,7 +1591,7 @@
   void fo^o() { return ; })cpp",
"fail: Couldn't find semicolon for target declaration."},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1585,7 +1649,7 @@
   void TARGET(){ return; }
   )cpp"},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,107 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap ParamToNewName;
+  llvm::DenseMap> RefLocs;
+  auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+// No need to rename if parameters already have the same name.
+if (DestParam->getName() == SourceParam->getName())
+  return;
+std::string NewName;
+// Unnamed parameters won't be visited in findExplicitReferences. So add
+// them here.
+if (DestParam->getName().empty()) {
+  RefLocs[DestParam].push_back(DestParam->getLocation());
+  // If decl is unnamed in destination we pad the new name to avoid gluing
+  // with previous token, e.g.

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-30 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, many thanks!




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:293
+  // Now try to generate edits for all the refs.
+  for (auto  : RefLocs) {
+const auto *OldDecl = Entry.first;

NIT: Maybe move declaration of `Replacements` here? To make it closer to the 
use-site.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-30 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-30 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 227035.
kadircet added a comment.

- Add comments
- Early return when generating edits fails


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -40,9 +40,9 @@
 #include 
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
-using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
@@ -1377,7 +1377,7 @@
   // Template body is not parsed until instantiation time on windows, which
   // results in arbitrary failures as function body becomes NULL.
   ExtraArgs.push_back("-fno-delayed-template-parsing");
-  for(const auto  : Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1420,7 +1420,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDeclRefs) {
-  auto Test =R"cpp(
+  auto Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -1489,6 +1489,71 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b, int T\
+est);
+
+void ^foo(int f, bool x, int z) {})cpp"),
+R"cpp(
+void foo(int f, bool x, int z){}
+
+)cpp");
+
+  EXPECT_EQ(apply(R"cpp(
+#define PARAM int Z
+void foo(PARAM);
+
+void ^foo(int X) {})cpp"),
+"fail: Cant rename parameter inside macro body.");
+
+  EXPECT_EQ(apply(R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+
+template 
+void ^foo(int Z, int b, int c, int d) {})cpp"),
+R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
@@ -1527,7 +1592,7 @@
   void fo^o() { return ; })cpp",
"fail: Couldn't find semicolon for target declaration."},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1585,7 +1650,7 @@
   void TARGET(){ return; }
   )cpp"},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,108 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  tooling::Replacements Replacements;
+
+  llvm::DenseMap ParamToNewName;
+  llvm::DenseMap> RefLocs;
+  auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+// No need to rename if parameters already have the same name.
+if (DestParam->getName() == SourceParam->getName())
+  return;
+std::string NewName;
+// Unnamed parameters won't be visited in findExplicitReferences. So add
+// them here.
+if (DestParam->getName().empty()) {
+  RefLocs[DestParam].push_back(DestParam->getLocation());
+  // If decl is unnamed in destination we pad the new name to avoid gluing
+  // with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+  NewName = " ";
+}
+NewName.append(SourceParam->getName());
+ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName);
+  };
+
+  // Populate mapping for

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226909.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Use Lexer::makeFileCharRange


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -40,9 +40,9 @@
 #include 
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
-using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
@@ -1377,7 +1377,7 @@
   // Template body is not parsed until instantiation time on windows, which
   // results in arbitrary failures as function body becomes NULL.
   ExtraArgs.push_back("-fno-delayed-template-parsing");
-  for(const auto  : Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1420,7 +1420,7 @@
 }
 
 TEST_F(DefineInlineTest, TransformDeclRefs) {
-  auto Test =R"cpp(
+  auto Test = R"cpp(
 namespace a {
   template  class Bar {
   public:
@@ -1489,6 +1489,71 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b, int T\
+est);
+
+void ^foo(int f, bool x, int z) {})cpp"),
+R"cpp(
+void foo(int f, bool x, int z){}
+
+)cpp");
+
+  EXPECT_EQ(apply(R"cpp(
+#define PARAM int Z
+void foo(PARAM);
+
+void ^foo(int X) {})cpp"),
+"fail: Cant rename parameter inside macro body.");
+
+  EXPECT_EQ(apply(R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+
+template 
+void ^foo(int Z, int b, int c, int d) {})cpp"),
+R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
@@ -1527,7 +1592,7 @@
   void fo^o() { return ; })cpp",
"fail: Couldn't find semicolon for target declaration."},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
@@ -1585,7 +1650,7 @@
   void TARGET(){ return; }
   )cpp"},
   };
-  for(const auto& Case: Cases)
+  for (const auto  : Cases)
 EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,117 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  tooling::Replacements Replacements;
+
+  llvm::DenseMap ParamToNewName;
+  llvm::DenseMap> RefLocs;
+  auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+// No need to rename if parameters already have the same name.
+if (DestParam->getName() == SourceParam->getName())
+  return;
+std::string NewName;
+// Unnamed parameters won't be visited in findExplicitReferences. So add
+// them here.
+if (DestParam->getName().empty()) {
+  RefLocs[DestParam].push_back(DestParam->getLocation());
+  // If decl is unnamed in destination we pad the new name to avoid gluing
+  // with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+  NewName = " ";
+}
+NewName.append(SourceParam->getName());
+ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName);
+  };
+
+  //

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1517
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+

ilya-biryukov wrote:
> We fail to rename the parameter in that case, right?
> Should the action not be available or maybe show an error message?
> 
> Also ok with keeping the current behavior if we add a FIXME mentioning it 
> would be nice to fix this.
somehow forgot my stashed changes ...

yes this was supposed to bail out on failure, but leave the macro occurrence if 
parameter names are the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1517
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+

We fail to rename the parameter in that case, right?
Should the action not be available or maybe show an error message?

Also ok with keeping the current behavior if we add a FIXME mentioning it would 
be nice to fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226868.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Handle macros in parameter names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -40,9 +40,9 @@
 #include 
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
-using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
@@ -1489,6 +1489,64 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b, int T\
+est);
+
+void ^foo(int f, bool x, int z) {})cpp"),
+R"cpp(
+void foo(int f, bool x, int z){}
+
+)cpp");
+
+  EXPECT_EQ(apply(R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+
+template 
+void ^foo(int a, int b, int c, int d) {})cpp"),
+R"cpp(
+#define TYPE int
+#define PARAM TYPE Z
+#define BODY(x) 5 * (x) + 2
+template 
+void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,105 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  tooling::Replacements Replacements;
+
+  llvm::DenseMap ParamToNewName;
+  llvm::DenseMap> RefLocs;
+  auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+// No need to rename if parameters already have the same name.
+if (DestParam->getName() == SourceParam->getName())
+  return;
+std::string NewName;
+// Unnamed parameters won't be visited in findExplicitReferences. So add
+// them here.
+if (DestParam->getName().empty()) {
+  RefLocs[DestParam].push_back(DestParam->getLocation());
+  // If decl is unnamed in destination we pad the new name to avoid gluing
+  // with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+  NewName = " ";
+}
+NewName.append(SourceParam->getName());
+ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName);
+  };
+
+  // Populate mapping for template parameters.
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (DestTempl) {
+const auto *DestTPL = DestTempl->getTemplateParameters();
+const auto *SourceTPL = SourceTempl->getTemplateParameters();
+assert(DestTPL->size() == SourceTPL->size());
+
+for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I)
+  HandleParam(DestTPL->getParam(I), SourceTPL->getParam(I));
+  }
+
+  // Populate mapping for function params.
+  assert(Dest->param_size() == Source->param_size());
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I)
+HandleParam(Dest->getParamDecl(I), Source->getParamDecl(I));
+
+  llvm::Error RenamingErrors = llvm::Error::success();
+  const SourceManager  = Dest->getASTContext().getSourceManager();
+  const LangOptions  = Dest->getASTContext().getLangOpts();
+  // Collect other references in function signature, i.e parameter types and
+  // default arguments.
+  findExplicitReferences(
+  // Use function template in case of templated functions to visit template
+

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

Just a few final NITs from my side. 
And would also be nice to get tests with macros.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:251
+}
+NewName.append(SourceParam->getName());
+ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName);

NIT: maybe use assignment syntax?
 `NewName += SourceParam->getName()`



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:270
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I)
+HandleParam(Dest->getParamDecl(I), Source->getParamDecl(I));
+

NIT: also add `assert(Source->param_size() == Dest->param_size())`?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:275
+  const LangOptions  = Dest->getASTContext().getLangOpts();
+  // Collect other references in function signautre, i.e parameter types and
+  // default arguments.

s/signautre/signature



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1492
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(

Could you add tests with some parameter references inside macro expansions?
To make sure we don't crash and don't produce invalid edits? Failing is 
obviously ok


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226849.
kadircet added a comment.

- Handle unnamed parameters explicitly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1489,6 +1489,45 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b, int T\
+est);
+
+void ^foo(int f, bool x, int z) {})cpp"), R"cpp(
+void foo(int f, bool x, int z){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,97 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  tooling::Replacements Replacements;
+
+  llvm::DenseMap ParamToNewName;
+  llvm::DenseMap> RefLocs;
+  auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+// No need to rename if parameters already have the same name.
+if (DestParam->getName() == SourceParam->getName())
+  return;
+std::string NewName;
+// Unnamed parameters won't be visited in findExplicitReferences. So add
+// them here.
+if (DestParam->getName().empty()) {
+  RefLocs[DestParam].push_back(DestParam->getLocation());
+  // If decl is unnamed in destination we pad the new name to avoid gluing
+  // with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+  NewName = " ";
+}
+NewName.append(SourceParam->getName());
+ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName);
+  };
+
+  // Populate mapping for template parameters.
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (DestTempl) {
+const auto *DestTPL = DestTempl->getTemplateParameters();
+const auto *SourceTPL = SourceTempl->getTemplateParameters();
+assert(DestTPL->size() == SourceTPL->size());
+
+for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I)
+  HandleParam(DestTPL->getParam(I), SourceTPL->getParam(I));
+  }
+
+  // Populate mapping for function params.
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I)
+HandleParam(Dest->getParamDecl(I), Source->getParamDecl(I));
+
+  llvm::Error RenamingErrors = llvm::Error::success();
+  const SourceManager  = Dest->getASTContext().getSourceManager();
+  const LangOptions  = Dest->getASTContext().getLangOpts();
+  // Collect other references in function signautre, i.e parameter types and
+  // default arguments.
+  findExplicitReferences(
+  // Use function template in case of templated functions to visit template
+  // parameters.
+  DestTempl ? llvm::dyn_cast(DestTempl) : llvm::dyn_cast(Dest),
+  [&](ReferenceLoc Ref) {
+if (Ref.Targets.size() != 1)
+  return;
+const auto *Target =
+llvm::cast(Ref.Targets.front()->getCanonicalDecl());
+auto It = ParamToNewName.find(Target);
+if (It == ParamToNewName.end())
+  return;
+RefLocs[Target].push_back(Ref.NameLoc);
+  });
+
+  for (auto  : RefLocs) {
+const auto *OldDecl = Entry.first;
+llvm::StringRef OldName = OldDecl->getName();
+llvm::StringRef NewName = ParamToNewName[OldDecl];
+for (const SourceLocation  : Entry.second) {
+  // Can't rely on OldName.size() because of multi-line identifiers, e.g.
+  // int T\
+

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: fail - 59657 tests passed, 2 failed and 805 were skipped.

  failed: 
libc++.libcxx/thread/thread_threads/thread_thread_this/sleep_for.pass.cpp
  failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:233
+bool fillTemplateParameterMapping(
+const FunctionDecl *Dest, const FunctionDecl *Source,
+llvm::DenseMap ) {

NIT: instead of returning whether `Dest` is a template, we could instead:
1. accept a `TemplateDecl* Dest` and `TemplateDecl *Source`,
2. check whether functions are template at the call site and only call 
`fillTemplateParameterMapping`  when they're template.

I believe that would make both functions simpler and more straight-forward. But 
up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

D69511  broke the anonymous parameters. Sorry 
about that, I hope that's for the best in the long run :-)
We'll need some code to update this patch. Other than that, I think this patch 
is good to go!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226655.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1489,6 +1489,45 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b, int T\
+est);
+
+void ^foo(int f, bool x, int z) {})cpp"), R"cpp(
+void foo(int f, bool x, int z){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,92 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Returns true if Dest is templated function. Fills in \p ParamToNewName with
+/// a mapping from template decls in \p Dest to theire respective names in \p
+/// Source.
+bool fillTemplateParameterMapping(
+const FunctionDecl *Dest, const FunctionDecl *Source,
+llvm::DenseMap ) {
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (!DestTempl)
+return false;
+  const auto *DestTPL = DestTempl->getTemplateParameters();
+  const auto *SourceTPL = SourceTempl->getTemplateParameters();
+  assert(DestTPL->size() == SourceTPL->size());
+
+  for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I)
+ParamToNewName[DestTPL->getParam(I)->getCanonicalDecl()] =
+SourceTPL->getParam(I)->getName();
+  return true;
+}
+
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap ParamToNewName;
+  bool HasTemplateParams =
+  fillTemplateParameterMapping(Dest, Source, ParamToNewName);
+  tooling::Replacements Replacements;
+
+  // Populate mapping for function params.
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I) {
+auto *DestParam = Dest->getParamDecl(I);
+auto *SourceParam = Source->getParamDecl(I);
+ParamToNewName[DestParam] = SourceParam->getName();
+  }
+
+  llvm::Error RenamingErrors = llvm::Error::success();
+  const SourceManager  = Dest->getASTContext().getSourceManager();
+  const LangOptions  = Dest->getASTContext().getLangOpts();
+  findExplicitReferences(
+  // Use function template in case of templated functions to visit template
+  // parameters.
+  HasTemplateParams
+  ? llvm::dyn_cast(Dest->getDescribedFunctionTemplate())
+  : llvm::dyn_cast(Dest),
+  [&](ReferenceLoc Ref) {
+if (Ref.Targets.size() != 1)
+  return;
+const auto *Target =
+llvm::cast(Ref.Targets.front()->getCanonicalDecl());
+auto It = ParamToNewName.find(Target);
+if (It == ParamToNewName.end())
+  return;
+// No need to rename if parameters already have the same name.
+if (Target->getName() == It->second)
+  return;
+// In case of an empty identifier, we can be sure about zero length, but
+// we need to lex to get token length otherwise because of multi-line
+// identifiers, e.g. int T\
+// est
+// We don't want to lex in the case of empty names, since the NameLoc
+// will still likely have some non-identifier token underneath, and it
+// will have some length, e.g. void foo(int, char c); for the first
+// parameter name will point at ','.
+const size_t OldNameLen =
+Target->getName().empty()
+? 0

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:278
+const auto *Target =
+llvm::dyn_cast(Ref.Targets.front()->getCanonicalDecl());
+auto It = ParamToNewName.find(Target);

why not `llvm::cast`? We'd rather fail early (during the cast) than postpone it 
until we dereference `Target` a few lines further



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:285
+  return;
+const size_t OldNameLen = Target->getName().size();
+// If decl is unnamed in destination we pad the new name to avoid 
gluing

Could we measure the token length instead?
There are various bizzare cases when the source text is different and we 
definitely don't want to accidentally crash on those, e.g.
```
int a = N\
A\
ME;
```



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:295
+  RenamingErrors =
+  llvm::joinErrors(std::move(RenamingErrors), std::move(Err));
+}

Wow, TIL I learned something new. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 59695 tests passed, 0 failed and 763 were skipped.
Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-28 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226619.
kadircet added a comment.

- Don't rename if parameters have the same name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1494,6 +1494,44 @@
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b);
+
+void ^foo(int f, bool x) {})cpp"), R"cpp(
+void foo(int f, bool x){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   auto Test = R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,80 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Returns true if Dest is templated function. Fills in \p ParamToNewName with
+/// a mapping from template decls in \p Dest to theire respective names in \p
+/// Source.
+bool fillTemplateParameterMapping(
+const FunctionDecl *Dest, const FunctionDecl *Source,
+llvm::DenseMap ) {
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (!DestTempl)
+return false;
+  const auto *DestTPL = DestTempl->getTemplateParameters();
+  const auto *SourceTPL = SourceTempl->getTemplateParameters();
+  assert(DestTPL->size() == SourceTPL->size());
+
+  for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I)
+ParamToNewName[DestTPL->getParam(I)->getCanonicalDecl()] =
+SourceTPL->getParam(I)->getName();
+  return true;
+}
+
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap ParamToNewName;
+  bool HasTemplateParams =
+  fillTemplateParameterMapping(Dest, Source, ParamToNewName);
+  tooling::Replacements Replacements;
+
+  // Populate mapping for function params.
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I) {
+auto *DestParam = Dest->getParamDecl(I);
+auto *SourceParam = Source->getParamDecl(I);
+ParamToNewName[DestParam] = SourceParam->getName();
+  }
+
+  llvm::Error RenamingErrors = llvm::Error::success();
+  const SourceManager  = Dest->getASTContext().getSourceManager();
+  findExplicitReferences(
+  // Use function template in case of templated functions to visit template
+  // parameters.
+  HasTemplateParams
+  ? llvm::dyn_cast(Dest->getDescribedFunctionTemplate())
+  : llvm::dyn_cast(Dest),
+  [&](ReferenceLoc Ref) {
+if (Ref.Targets.size() != 1)
+  return;
+const auto *Target =
+llvm::dyn_cast(Ref.Targets.front()->getCanonicalDecl());
+auto It = ParamToNewName.find(Target);
+if (It == ParamToNewName.end())
+  return;
+// No need to rename if parameters already have the same name.
+if (Target->getName() == It->second)
+  return;
+const size_t OldNameLen = Target->getName().size();
+// If decl is unnamed in destination we pad the new name to avoid gluing
+// with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+auto ReplacementText = (OldNameLen == 0 ? " " : "") + It->second;
+if (auto Err = Replacements.add(tooling::Replacement(
+SM, Ref.NameLoc, OldNameLen, ReplacementText))) {
+  elog("define inline: Couldn't replace parameter name for {0} to {1}: "
+   "{2}",
+   Target->getName(), ReplacementText, Err);
+  RenamingErrors =
+  llvm::joinErrors(std::move(RenamingErrors),

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-25 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: fail - 59631 tests passed, 1 failed and 805 were skipped.

  failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-25 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226517.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Use findExplicitReferences for decl traversals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1469,6 +1469,44 @@
 )cpp");
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b);
+
+void ^foo(int f, bool x) {})cpp"), R"cpp(
+void foo(int f, bool x){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   EXPECT_EQ(apply(R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,80 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Returns true if Dest is templated function. Fills in \p ParamToNewName with
+/// a mapping from template decls in \p Dest to theire respective names in \p
+/// Source.
+bool fillTemplateParameterMapping(
+const FunctionDecl *Dest, const FunctionDecl *Source,
+llvm::DenseMap ) {
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (!DestTempl)
+return false;
+  const auto *DestTPL = DestTempl->getTemplateParameters();
+  const auto *SourceTPL = SourceTempl->getTemplateParameters();
+  assert(DestTPL->size() == SourceTPL->size());
+
+  for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I)
+ParamToNewName[DestTPL->getParam(I)->getCanonicalDecl()] =
+SourceTPL->getParam(I)->getName();
+  return true;
+}
+
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap ParamToNewName;
+  bool HasTemplateParams =
+  fillTemplateParameterMapping(Dest, Source, ParamToNewName);
+  tooling::Replacements Replacements;
+
+  // Populate mapping for function params.
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I) {
+auto *DestParam = Dest->getParamDecl(I);
+auto *SourceParam = Source->getParamDecl(I);
+ParamToNewName[DestParam] = SourceParam->getName();
+  }
+
+  llvm::Error RenamingErrors = llvm::Error::success();
+  const SourceManager  = Dest->getASTContext().getSourceManager();
+  findExplicitReferences(
+  // Use function template in case of templated functions to visit template
+  // parameters.
+  HasTemplateParams
+  ? llvm::dyn_cast(Dest->getDescribedFunctionTemplate())
+  : llvm::dyn_cast(Dest),
+  [&](ReferenceLoc Ref) {
+if (Ref.Targets.size() != 1)
+  return;
+const auto *Target =
+llvm::dyn_cast(Ref.Targets.front()->getCanonicalDecl());
+auto It = ParamToNewName.find(Target);
+if (It == ParamToNewName.end())
+  return;
+const size_t OldNameLen = Target->getName().size();
+// If decl is unnamed in both source and destination, just skip.
+if (OldNameLen == 0 && It->second.size() == 0)
+  return;
+// If decl is unnamed in destination we pad the new name to avoid gluing
+// with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+auto ReplacementText = (OldNameLen == 0 ? " " : "") + It->second;
+if (auto Err = Replacements.add(tooling::Replacement(
+SM, Ref.NameLoc, OldNameLen, ReplacementText))) {
+  elog("define inline: Couldn't replace parameter name for {0} to {1}: "
+   "{2}",
+   Target->getName(), ReplacementText, Err);
+  RenamingErrors =
+

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-25 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

Ah, we're actually trying to rename parameters in the declaration to match the 
ones in the definition... So the try-catch blocks are not a problem, actually


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-25 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

In D68937#1710915 , @kadircet wrote:

> I totally agree that the solution you proposed would also work, but don't 
> think it would be any less code. Since one needs to correlate
>  parameters between two different declarations, and `findExplicitReferences` 
> doesn't really provide a nice way to achieve that. One
>  would have to rely on `SourceLocation` ordering or in the order callback was 
> issued(which implicitly relies on AST traversal order).
>
> It would be nice to unify the rewrite parameter name/type/defaultarg logics, 
> but not sure if it is worth.


I believe it should be less and much simpler code. In particular, I propose 
something like this:

  DenseSet ParametersToRename;
  // Fill ParametersToRename with template and function parameters.
  
  DenseMap> References;
  findExplicitReferences(Func, [&](ReferenceLoc R) {
if (!ParametersToRename.count(R.Target))
  return;
References[R.Target].push_back(R.NameLoc);
  });
  
  for (auto [Param, Locations] : References) {
auto NewName = NewNameForParam(Param);
for (auto L : Locations)
  Replacements.add(replaceToken(L, NewName));
  }

That should also handle various interesting cases like `try-catch` blocks and 
constructor initializer lists.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:229
 
+/// Generates a replacement that renames \p Dest to \p Source.
+llvm::Error rewriteParameterName(tooling::Replacements ,

NIT: `that renames \p Dest to \p SourceName`



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:232
+ const NamedDecl *Dest,
+ llvm::StringRef SourceName) {
+  auto  = Dest->getASTContext().getSourceManager();

NIT: call it `NewName`?
`Source` and `Target` are very define-inline-specific. This function actually 
does a more general thing and using the define-inline terminology hurt the 
redability a little.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:239
+  SM, Dest->getLocation(), DestName.size(),
+  // If \p Dest is unnamed, we need to insert a space before current
+  // name.

Why do we need to insert a space here? could you add an example?
I guess it's 
```
void foo(int^) // <-- avoid gluing 'int' and the new name here
```



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:241
+  // name.
+  ((DestName.empty() ? " " : "") + SourceName).str( {
+return Err;

NIT: could you move this expression and a comment to a separate variable?
should simplify the if condition and improve readability


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-25 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 226389.
kadircet added a comment.

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1469,6 +1469,44 @@
 )cpp");
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b);
+
+void ^foo(int f, bool x) {})cpp"), R"cpp(
+void foo(int f, bool x){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   EXPECT_EQ(apply(R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,176 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates a replacement that renames \p Dest to \p Source.
+llvm::Error rewriteParameterName(tooling::Replacements ,
+ const NamedDecl *Dest,
+ llvm::StringRef SourceName) {
+  auto  = Dest->getASTContext().getSourceManager();
+  llvm::StringRef DestName = Dest->getName();
+  if (DestName == SourceName)
+return llvm::Error::success();
+  if (auto Err = Replacements.add(tooling::Replacement(
+  SM, Dest->getLocation(), DestName.size(),
+  // If \p Dest is unnamed, we need to insert a space before current
+  // name.
+  ((DestName.empty() ? " " : "") + SourceName).str( {
+return Err;
+  }
+  return llvm::Error::success();
+}
+
+/// Returns a mapping from template names in \p Dest to \p Source, so that they
+/// can be updated in other places like parameter types and default arguments.
+llvm::Expected>
+renameTemplateParameters(tooling::Replacements ,
+ const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap TemplParamNameDestToSource;
+
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (DestTempl) {
+const auto *DestTPL = DestTempl->getTemplateParameters();
+const auto *SourceTPL = SourceTempl->getTemplateParameters();
+assert(DestTPL->size() == SourceTPL->size());
+
+for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I) {
+  if (auto Err = rewriteParameterName(Replacements, DestTPL->getParam(I),
+  SourceTPL->getParam(I)->getName()))
+return std::move(Err);
+  TemplParamNameDestToSource[DestTPL->getParam(I)->getName()] =
+  SourceTPL->getParam(I)->getName();
+}
+  }
+
+  return TemplParamNameDestToSource;
+}
+
+/// Generates replacements for rewriting dependent types in \p DestParam to the
+/// matching template name in \p TemplParamNameDestToSource.
+llvm::Error
+rewriteParameterType(tooling::Replacements ,
+ const ParmVarDecl *DestParam,
+ const llvm::DenseMap
+ ) {
+  auto *DestTSI = DestParam->getTypeSourceInfo();
+  if (!DestTSI)
+return llvm::Error::success();
+  const SourceManager  = DestParam->getASTContext().getSourceManager();
+
+  // Update paramater types if they are template template or type template.
+  auto DestTL = DestTSI->getTypeLoc();
+  if (auto DestTPTL = DestTL.getAs()) {
+if (auto Err = Replacements.add(tooling::Replacement(
+SM, CharSourceRange(DestTPTL.getSourceRange(), /*ITR=*/true),
+TemplParamNameDestToSource.lookup(
+DestTPTL.getDecl()->getDeclName().getAsString() {
+  return Err;
+}
+  } else if (auto DestTTSL = DestTL.getAs()) {
+std::string TemplName;
+llvm::raw_string_ostream OS(TemplName);
+DestTTSL.getTypePtr()->getTemplateName().print(
+OS,

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-16 Thread Christian Kühnel via Phabricator via cfe-commits 
kuhnel added a comment.

Build failed as patch failed to apply:

  00:00:06.098 Checking patch clang-tools-extra/trunk/clangd/FindTarget.h...
  00:00:06.100 error: clang-tools-extra/trunk/clangd/FindTarget.h: does not 
exist in index
  00:00:06.100 Checking patch clang-tools-extra/trunk/clangd/FindTarget.cpp...
  00:00:06.100 error: clang-tools-extra/trunk/clangd/FindTarget.cpp: does not 
exist in index
  00:00:06.101 
  00:00:06.101  Patch Failed! 
  00:00:06.101 Usage Exception: Unable to apply patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-16 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet updated this revision to Diff 225209.
kadircet marked an inline comment as done.
kadircet added a comment.

- Accept StringRef for Source in rewriteParameterName instead of a NamedDecl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1290,6 +1290,44 @@
 )cpp");
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b);
+
+void ^foo(int f, bool x) {})cpp"), R"cpp(
+void foo(int f, bool x){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   EXPECT_EQ(apply(R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,176 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates a replacement that renames \p Dest to \p Source.
+llvm::Error rewriteParameterName(tooling::Replacements ,
+ const NamedDecl *Dest,
+ llvm::StringRef SourceName) {
+  auto  = Dest->getASTContext().getSourceManager();
+  llvm::StringRef DestName = Dest->getName();
+  if (DestName == SourceName)
+return llvm::Error::success();
+  if (auto Err = Replacements.add(tooling::Replacement(
+  SM, Dest->getLocation(), DestName.size(),
+  // If \p Dest is unnamed, we need to insert a space before current
+  // name.
+  ((DestName.empty() ? " " : "") + SourceName).str( {
+return Err;
+  }
+  return llvm::Error::success();
+}
+
+/// Returns a mapping from template names in \p Dest to \p Source, so that they
+/// can be updated in other places like parameter types and default arguments.
+llvm::Expected>
+renameTemplateParameters(tooling::Replacements ,
+ const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap TemplParamNameDestToSource;
+
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (DestTempl) {
+const auto *DestTPL = DestTempl->getTemplateParameters();
+const auto *SourceTPL = SourceTempl->getTemplateParameters();
+assert(DestTPL->size() == SourceTPL->size());
+
+for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I) {
+  if (auto Err = rewriteParameterName(Replacements, DestTPL->getParam(I),
+  SourceTPL->getParam(I)->getName()))
+return std::move(Err);
+  TemplParamNameDestToSource[DestTPL->getParam(I)->getName()] =
+  SourceTPL->getParam(I)->getName();
+}
+  }
+
+  return TemplParamNameDestToSource;
+}
+
+/// Generates replacements for rewriting dependent types in \p DestParam to the
+/// matching template name in \p TemplParamNameDestToSource.
+llvm::Error
+rewriteParameterType(tooling::Replacements ,
+ const ParmVarDecl *DestParam,
+ const llvm::DenseMap
+ ) {
+  auto *DestTSI = DestParam->getTypeSourceInfo();
+  if (!DestTSI)
+return llvm::Error::success();
+  const SourceManager  = DestParam->getASTContext().getSourceManager();
+
+  // Update paramater types if they are template template or type template.
+  auto DestTL = DestTSI->getTypeLoc();
+  if (auto DestTPTL = DestTL.getAs()) {
+if (auto Err = Replacements.add(tooling::Replacement(
+SM, CharSourceRange(DestTPTL.getSourceRange(), /*ITR=*/true),
+TemplParamNameDestToSource.lookup(
+DestTPTL.getDecl()->getDeclName().getAsString() {
+  return Err;
+}
+  } else if (auto DestTTSL = DestTL.getAs()) {
+std::string TemplName;
+llvm::raw_string_ostream OS(TemplName);

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-16 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added a comment.

In D68937#1710634 , @ilya-biryukov 
wrote:

> An alternative approach I'm thinking of:
>  After D68977  lands, we could try using 
> `findExplicitReferences` to produce all of these edits:
>
> 1. we collect locations of all references and declaration of relevant 
> parameters and template parameters.
> 2. find the ones where the name differs and produce the changes.
>
>   That would handle not only references in the default argument, but also all 
> references in the body and should be less code overall. IMO it's worth 
> exploring this approach right away, e.g. you could layer your patch on top of 
> the current version of D68977 


I totally agree that the solution you proposed would also work, but don't think 
it would be any less code. Since one needs to correlate
parameters between two different declarations, and `findExplicitReferences` 
doesn't really provide a nice way to achieve that. One
would have to rely on `SourceLocation` ordering or in the order callback was 
issued(which implicitly relies on AST traversal order).

It would be nice to unify the rewrite parameter name/type/defaultarg logics, 
but not sure if it is worth.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-16 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

An alternative approach I'm thinking of:
After D68977  lands, we could try using 
`findExplicitReferences` to produce all of these edits:

1. we collect locations of all references and declaration of relevant 
parameters and template parameters.
2. find the ones where the name differs and produce the changes.

That would handle not only references in the default argument, but also all 
references in the body and should be less code overall.
IMO it's worth exploring this approach right away, e.g. you could layer your 
patch on top of the current version of D68977 




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:229
 
+/// Generates a replacement that renames \p Dest to \p Source.
+llvm::Error rewriteParameterName(tooling::Replacements ,

We only need name from the `Source`. Why not accept it as a `StringRef Name` 
directly?
Would allow to call this function even if `Source` is not a `NamedDecl`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937



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

[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

2019-10-14 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay.
Herald added a project: clang.

When moving a function definition to declaration location we also need
to handle renaming of the both function and template parameters.

This patch achives that by making sure every parameter name and dependent type
in destination is renamed to their respective name in the source.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68937

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1285,6 +1285,44 @@
 )cpp");
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+void foo(int, bool b);
+
+void ^foo(int f, bool x) {})cpp"), R"cpp(
+void foo(int f, bool x){}
+
+)cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+struct Foo {
+  struct Bar {
+template  class, template class Y,
+  int, int Z>
+void foo(X, Y, int W = 5 * Z + 2);
+  };
+};
+
+template  class V, template class W,
+  int X, int Y>
+void Foo::Bar::f^oo(U, W, int Q) {})cpp"),
+R"cpp(
+struct Foo {
+  struct Bar {
+template  class V, template class W,
+  int X, int Y>
+void foo(U, W, int Q = 5 * Y + 2){}
+  };
+};
+
+)cpp");
+}
+
 TEST_F(DefineInlineTest, TransformInlineNamespaces) {
   EXPECT_EQ(apply(R"cpp(
 namespace a { inline namespace b { namespace { struct Foo{}; } } }
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,176 @@
   return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
 }
 
+/// Generates a replacement that renames \p Dest to \p Source.
+llvm::Error rewriteParameterName(tooling::Replacements ,
+ const NamedDecl *Dest,
+ const NamedDecl *Source) {
+  auto  = Dest->getASTContext().getSourceManager();
+  llvm::StringRef DestName = Dest->getName();
+  llvm::StringRef SourceName = Source->getName();
+  if (DestName == SourceName)
+return llvm::Error::success();
+  if (auto Err = Replacements.add(tooling::Replacement(
+  SM, Dest->getLocation(), DestName.size(),
+  // If \p Dest is unnamed, we need to insert a space before current
+  // name.
+  ((DestName.empty() ? " " : "") + SourceName).str( {
+return Err;
+  }
+  return llvm::Error::success();
+}
+
+/// Returns a mapping from template names in \p Dest to \p Source, so that they
+/// can be updated in other places like parameter types and default arguments.
+llvm::Expected>
+renameTemplateParameters(tooling::Replacements ,
+ const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap TemplParamNameDestToSource;
+
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+  if (DestTempl) {
+const auto *DestTPL = DestTempl->getTemplateParameters();
+const auto *SourceTPL = SourceTempl->getTemplateParameters();
+assert(DestTPL->size() == SourceTPL->size());
+
+for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I) {
+  if (auto Err = rewriteParameterName(Replacements, DestTPL->getParam(I),
+  SourceTPL->getParam(I)))
+return std::move(Err);
+  TemplParamNameDestToSource[DestTPL->getParam(I)->getName()] =
+  SourceTPL->getParam(I)->getName();
+}
+  }
+
+  return TemplParamNameDestToSource;
+}
+
+/// Generates replacements for rewriting dependent types in \p DestParam to the
+/// matching template name in \p TemplParamNameDestToSource.
+llvm::Error
+rewriteParameterType(tooling::Replacements ,
+ const ParmVarDecl *DestParam,
+ const llvm::DenseMap
+ ) {
+  auto *DestTSI = DestParam->getTypeSourceInfo();
+  if (!DestTSI)
+return llvm::Error::success();
+  const SourceManager  = DestParam->getASTContext().getSourceManager();
+
+  // Update paramater types if they are template template or type template.
+  auto DestTL = DestTSI->getTypeLoc();
+  if (auto DestTPTL = DestTL.getAs()) {
+if (auto Err = Replacements.add(tooling::Replacement(
+SM, CharSourceRange(DestTPTL.getSourceRange(), /*ITR=*/true),
+