[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdfd6374c784f: [clangd] DefineInline action apply logic with 
fully qualified names (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -40,11 +41,16 @@
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
+using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
 TEST(FileEdits, AbsolutePath) {
   auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
 
@@ -1047,6 +1053,512 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformNestedNamespaces) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo();
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  void f^oo() {
+bar();
+a::bar();
+
+baz();
+b::baz();
+a::b::baz();
+
+aux();
+c::aux();
+b::c::aux();
+a::b::c::aux();
+  })cpp"), R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo(){
+a::bar();
+a::bar();
+
+a::b::baz();
+a::b::baz();
+a::b::baz();
+
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+using namespace b;
+using namespace c;
+using c::aux;
+namespace d = c;
+  })cpp"),
+R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo(){
+using namespace a;
+using namespace a::b;
+using namespace a::b::c;
+using a::b::c::aux;
+namespace d = a::b::c;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo();
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  })cpp"),
+R"cpp(
+  void foo(){
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo();
+
+  using namespace a;
+  void f^oo() {
+bar>.bar();
+aux>();
+  })cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo(){
+a::bar>.bar();
+a::aux>();
+  }
+
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo();
+  };
+
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };
+
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo();
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 226388.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- s/SemiColon/Semicolon/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -40,11 +41,16 @@
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
+using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
 TEST(FileEdits, AbsolutePath) {
   auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
 
@@ -1047,6 +1053,512 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformNestedNamespaces) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo();
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  void f^oo() {
+bar();
+a::bar();
+
+baz();
+b::baz();
+a::b::baz();
+
+aux();
+c::aux();
+b::c::aux();
+a::b::c::aux();
+  })cpp"), R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo(){
+a::bar();
+a::bar();
+
+a::b::baz();
+a::b::baz();
+a::b::baz();
+
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+using namespace b;
+using namespace c;
+using c::aux;
+namespace d = c;
+  })cpp"),
+R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo(){
+using namespace a;
+using namespace a::b;
+using namespace a::b::c;
+using a::b::c::aux;
+namespace d = a::b::c;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo();
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  })cpp"),
+R"cpp(
+  void foo(){
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo();
+
+  using namespace a;
+  void f^oo() {
+bar>.bar();
+aux>();
+  })cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo(){
+a::bar>.bar();
+a::aux>();
+  }
+
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo();
+  };
+
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };
+
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo();
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {};
+  }
+
+  template 
+  void 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:339
+
+const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+   *QualifiedBody);

s/SemiColon/Semicolon



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:323
+
+auto SemiColon = getSemicolonForDecl(Target);
+if (!SemiColon) {

ilya-biryukov wrote:
> NIT: s/SemiColon/Semicolon
This one is still there.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1332
+  template <>
+  void fo^o(char p) {
+return;

kadircet wrote:
> ilya-biryukov wrote:
> > How is this test different from the previous one?
> > Is it just trying to make sure we don't always move to the first function 
> > body?
> yes that's the point, adding a comment.
Thanks!



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1374
+  )cpp");
+}
+

kadircet wrote:
> ilya-biryukov wrote:
> > Do we test the following case anywhere?
> > ```
> > template  void foo();
> > template <> void ^foo() { // should not be available
> >   return;
> > }
> > ```
> Yes, there is once check in availability tests for:
> ```
>   EXPECT_UNAVAILABLE(R"cpp(
> template  void foo();
> 
> template<> void f^oo() {
> })cpp");
> ```
Ah, nice, I missed it. Thanks for pointing it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:334
+
+const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+   *QualifiedBody);

ilya-biryukov wrote:
> Could we add a test when the semicolon comes from a macro expansion? I 
> believe the action won't be available in that case, which is ok. Just to make 
> sure we cover this corner-case.
> 
> ```
> #define SEMI ;
> void foo() SEMI
> ```
> 
> Also interesting cases:
> - source function is in a macro expansion
> - target function is in a macro expansion.
> 
> I believe they might catch a few bugs here, as we seem to assume all 
> locations are file locations...
see macro tests for the behavior in such cases.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1332
+  template <>
+  void fo^o(char p) {
+return;

ilya-biryukov wrote:
> How is this test different from the previous one?
> Is it just trying to make sure we don't always move to the first function 
> body?
yes that's the point, adding a comment.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1374
+  )cpp");
+}
+

ilya-biryukov wrote:
> Do we test the following case anywhere?
> ```
> template  void foo();
> template <> void ^foo() { // should not be available
>   return;
> }
> ```
Yes, there is once check in availability tests for:
```
  EXPECT_UNAVAILABLE(R"cpp(
template  void foo();

template<> void f^oo() {
})cpp");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 226230.
kadircet marked 15 inline comments as done.
kadircet added a comment.

- Improve macro handling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -40,11 +41,16 @@
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
+using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
 TEST(FileEdits, AbsolutePath) {
   auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
 
@@ -1047,6 +1053,512 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformNestedNamespaces) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo();
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  void f^oo() {
+bar();
+a::bar();
+
+baz();
+b::baz();
+a::b::baz();
+
+aux();
+c::aux();
+b::c::aux();
+a::b::c::aux();
+  })cpp"), R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo(){
+a::bar();
+a::bar();
+
+a::b::baz();
+a::b::baz();
+a::b::baz();
+
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+using namespace b;
+using namespace c;
+using c::aux;
+namespace d = c;
+  })cpp"),
+R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo(){
+using namespace a;
+using namespace a::b;
+using namespace a::b::c;
+using a::b::c::aux;
+namespace d = a::b::c;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo();
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  })cpp"),
+R"cpp(
+  void foo(){
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo();
+
+  using namespace a;
+  void f^oo() {
+bar>.bar();
+aux>();
+  })cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo(){
+a::bar>.bar();
+a::aux>();
+  }
+
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo();
+  };
+
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };
+
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo();
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {};
+  }
+
+  template 
+  void 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

A few last NITs and one important comment about handling the case when function 
definition come from macro expansions




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:323
+
+auto SemiColon = getSemicolonForDecl(Target);
+if (!SemiColon) {

NIT: s/SemiColon/Semicolon



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:334
+
+const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+   *QualifiedBody);

Could we add a test when the semicolon comes from a macro expansion? I believe 
the action won't be available in that case, which is ok. Just to make sure we 
cover this corner-case.

```
#define SEMI ;
void foo() SEMI
```

Also interesting cases:
- source function is in a macro expansion
- target function is in a macro expansion.

I believe they might catch a few bugs here, as we seem to assume all locations 
are file locations...



Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:73
+  //for the main file.
+  //Populates \p EditedFiles if there were changes to other files whenever
+  //it is non-null. It is a mapping from absolute path of the edited file 
to

Hm, maybe even call `ADD_FAILURE()` when there are changes to other files and 
tests pass `null` to `EditedFiles`?
Likely an error in the test.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1226
+void foo(){
+return;
+  }

Argh... Without `clang-format` that looks SO weird :-)



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1266
+  template 
+  void foo();
+

Maybe put it before `using  namespace a;`?
To make sure the test does not need to change if we actually support not 
qualifying when not neccessary



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1332
+  template <>
+  void fo^o(char p) {
+return;

How is this test different from the previous one?
Is it just trying to make sure we don't always move to the first function body?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1374
+  )cpp");
+}
+

Do we test the following case anywhere?
```
template  void foo();
template <> void ^foo() { // should not be available
  return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[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:208
+
+  if (HadErrors) {
+return llvm::createStringError(

kadircet wrote:
> ilya-biryukov wrote:
> > NIT: braces are redundant
> I've thought you were a fan of braces when the inner statement spans multiple 
> lines, even if it is a single statement :D
It's more complicated than that... Let me explain...
I'm a fan of braces when:
1. There are line comments (I perceive those as an extra statement):
```
if (something) {
  // Here comes a comment...
  return 10;
}
```
2. There are composite statements inside the if body:
```
if (something) {
  for (;;)
do_something_else();
}
```

In your case, I'd not use braces...
It's not important, though, pick the style you like most.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+template  class Bar {};
+Bar z;

kadircet wrote:
> ilya-biryukov wrote:
> > Template declarations inside function bodies are not allowed in C++.
> > Are we getting compiler errors here and not actually testing anything?
> well, it was still working and inlining the function definition, apparently 
> despite the diagnostic, ranges were correct.
I guess we didn't need to do any edits there, so we just carried the text 
unchanged.
Had we actually needed to change something inside the template, we'd fail to do 
so.

Range of the function body is correct, since the parser can easily recover in 
those cases.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1215
+namespace b {
+class Foo{};
+namespace c {

kadircet wrote:
> ilya-biryukov wrote:
> > Could you indent here and in other tests? it's hard to follow the fact that 
> > `Foo` is inside `b` without indentation.
> > Same for other tests.
> > 
> > Alternatively, put on one line if there's just a single declaration inside 
> > the namespace
> no more nested namespaces
Yay! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79
+  Token CurToken;
+  while (!CurToken.is(tok::semi)) {
+if (RawLexer.LexFromRawLexer(CurToken))

kadircet wrote:
> ilya-biryukov wrote:
> > What are the tokens we expect to see before the semicolon here?
> > It feels safer to just skip comments and whitespace and check the next 
> > token is a semicolon.
> > 
> > Any reason to have an actual loop here?
> it has been a long time since our offline discussions around this one. but 
> the idea was could have any attributes before semicolon, these could be 
> macros that will expand to one thing in some platforms and another in a 
> different platform. Therefore we've decided to skip anything until we've 
> found a semicolon.
> 
> I don't think I follow the second part of the question though? What do you 
> mean by "having an actual loop"?
The loop I mentioned is 
```while (!CurToken.is(tok::semi))```
I believe this could be replaced with
```
auto NextTok = Lexer::findNextToken(...);
if (NextTok && NextTok->is(tok::semi))
  ...
```

There are two common attribute applications we should mostly care about: 
```
// Attribute in declaration specifiers.
[[noreturn]] int foo(); 

// Attribute after declarator name
int foo [[noreturn]]();
```
Both work with a simple if statement, rather than the attributes.

The case we're covering with the loop is super uncommon and I don't think we 
should even bother to support it:
```
int foo() [[some::attribute]];
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 226209.
kadircet marked 20 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -40,11 +41,16 @@
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
+using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
 TEST(FileEdits, AbsolutePath) {
   auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
 
@@ -1047,6 +1053,455 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformNestedNamespaces) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo();
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  void f^oo() {
+bar();
+a::bar();
+
+baz();
+b::baz();
+a::b::baz();
+
+aux();
+c::aux();
+b::c::aux();
+a::b::c::aux();
+  })cpp"), R"cpp(
+  namespace a {
+void bar();
+namespace b {
+  void baz();
+  namespace c {
+void aux();
+  }
+}
+  }
+
+  void foo(){
+a::bar();
+a::bar();
+
+a::b::baz();
+a::b::baz();
+a::b::baz();
+
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+a::b::c::aux();
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+using namespace b;
+using namespace c;
+using c::aux;
+namespace d = c;
+  })cpp"),
+R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo(){
+using namespace a;
+using namespace a::b;
+using namespace a::b::c;
+using a::b::c::aux;
+namespace d = a::b::c;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo();
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  })cpp"),
+R"cpp(
+  void foo(){
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo();
+
+  using namespace a;
+  void f^oo() {
+bar>.bar();
+aux>();
+  })cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo(){
+a::bar>.bar();
+a::aux>();
+  }
+
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo();
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo();
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo(){
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {};
+  }
+  using namespace a;

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79
+  Token CurToken;
+  while (!CurToken.is(tok::semi)) {
+if (RawLexer.LexFromRawLexer(CurToken))

ilya-biryukov wrote:
> What are the tokens we expect to see before the semicolon here?
> It feels safer to just skip comments and whitespace and check the next token 
> is a semicolon.
> 
> Any reason to have an actual loop here?
it has been a long time since our offline discussions around this one. but the 
idea was could have any attributes before semicolon, these could be macros that 
will expand to one thing in some platforms and another in a different platform. 
Therefore we've decided to skip anything until we've found a semicolon.

I don't think I follow the second part of the question though? What do you mean 
by "having an actual loop"?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208
+
+  if (HadErrors) {
+return llvm::createStringError(

ilya-biryukov wrote:
> NIT: braces are redundant
I've thought you were a fan of braces when the inner statement spans multiple 
lines, even if it is a single statement :D



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+template  class Bar {};
+Bar z;

ilya-biryukov wrote:
> Template declarations inside function bodies are not allowed in C++.
> Are we getting compiler errors here and not actually testing anything?
well, it was still working and inlining the function definition, apparently 
despite the diagnostic, ranges were correct.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1076
+  template <>
+  void foo(int p);
+

ilya-biryukov wrote:
> This is really uncommon, I'm not even sure we should test this.
> The other case is much more common and interesting:
> 
> ```
> template 
> void foo(T p);
> 
> template <>
> void foo(int p) { /* do something */ }
> 
> ```
> 
> Could we add a test that we don't suggest this action in that case?
yes there is a test for this in availability checks patch:

```
EXPECT_UNAVAILABLE(R"cpp(
template  void foo();

template<> void f^oo() {
})cpp");
```



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1215
+namespace b {
+class Foo{};
+namespace c {

ilya-biryukov wrote:
> Could you indent here and in other tests? it's hard to follow the fact that 
> `Foo` is inside `b` without indentation.
> Same for other tests.
> 
> Alternatively, put on one line if there's just a single declaration inside 
> the namespace
no more nested namespaces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly comments about tests, the implementation itself LG.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68
 
+llvm::Optional getSemiColonForDecl(const FunctionDecl *FD) {
+  const SourceManager  = FD->getASTContext().getSourceManager();

NIT: s/SemiColon/Semicolon



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79
+  Token CurToken;
+  while (!CurToken.is(tok::semi)) {
+if (RawLexer.LexFromRawLexer(CurToken))

What are the tokens we expect to see before the semicolon here?
It feels safer to just skip comments and whitespace and check the next token is 
a semicolon.

Any reason to have an actual loop here?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:181
+  if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
+elog("Targets from multiple contexts: {0}, {1}",
+ printQualifiedName(*Ref.Targets.front()), 
printQualifiedName(*ND));

NIT: could we mention the tweak name here? to make sure it's easy to detect 
those lines in the logs:
`define inline: targets from multiple contexts`



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208
+
+  if (HadErrors) {
+return llvm::createStringError(

NIT: braces are redundant



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:320
   Expected apply(const Selection ) override {
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Not implemented yet");
+const ASTContext  = Sel.AST.getASTContext();
+const SourceManager  = AST.getSourceManager();

NIT: use auto 



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:325
+if (!SemiColon) {
+  return llvm::createStringError(
+  llvm::inconvertibleErrorCode(),

NIT: braces are redundant



Comment at: clang-tools-extra/clangd/unittests/TweakTesting.cpp:85
+std::string TweakTest::apply(llvm::StringRef MarkedCode,
+ llvm::StringMap *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);

Could you document `EditedFiles` contains all other edited files and the 
original `std::string` only contains results for the main file?
Would also be useful to document what is the key in the StringMap: 
absolute/relative paths or URI?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:905
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+

Why have a comment in this test?
If we need to test we handle comments before the semicolon, could we move to a 
separate focused test?
If we want to test something else, let's leave out the comment. The test is 
hard enough to read on its own



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+template  class Bar {};
+Bar z;

Template declarations inside function bodies are not allowed in C++.
Are we getting compiler errors here and not actually testing anything?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:961
+
+  void foo()/*Comment -_-*/  ;
+

Same here: we don't need the comment here.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:964
+  void f^oo() {
+using namespace a;
+bar>.bar();

Either remove `using namespace a` from the function body or put a FIXME this 
shouldn't actually qualify?
I believe the first option is what we want, if we're trying to check template 
arguments are getting transformed properly



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:989
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;

Maybe move this matcher to the start of the file?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1029
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(

We prefer to have `using ::testing::ElementsAre` at the start of the file and 
not qualify the usage everywhere in our tests.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1076
+  template <>
+  void foo(int p);
+

This is really uncommon, I'm not even sure we should test this.
The other case is much more common and interesting:

```
template 
void foo(T p);

template <>
void foo(int p) { /* do something */ }

```

Could we add a test that we don't suggest this action in that case?



Comment at: 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:226
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  const SourceManager  = Dest->getASTContext().getSourceManager();

kadircet wrote:
> ilya-biryukov wrote:
> > This does not rename any references to those parameters, right?
> > E.g.
> > ```
> > template  void foo(T x);
> > 
> > template  void foo(U x) {}
> > ```
> > 
> > would turn into
> > ```
> > template  void foo(T x);
> > ```
> > right?
> yes that's right, just adding a unittest will address this later on
moving into D68937, since it got bigger than expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Move parameter renaming logic to a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -826,6 +827,477 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196
+
+std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+if (auto Err = Replacements.add(

ilya-biryukov wrote:
> You would want to use `ND->printNestedNameSpecifier()` instead to avoid 
> printing inline namespaces:
> ```
> namespace std { inline namespace v1 { 
>  struct vector {};
> }}
> ```
> 
> ^-- I believe the current code would print `std::v1::` for `vector` and we 
> want `std::`.
> Could you add this example to tests too?
it is the opposite, `printNamespaceScope` skips anonymous and inline 
namespaces, whereas `printNestedNameSpecifier` also prints those. adding a test.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:226
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  const SourceManager  = Dest->getASTContext().getSourceManager();

ilya-biryukov wrote:
> This does not rename any references to those parameters, right?
> E.g.
> ```
> template  void foo(T x);
> 
> template  void foo(U x) {}
> ```
> 
> would turn into
> ```
> template  void foo(T x);
> ```
> right?
yes that's right, just adding a unittest will address this later on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 223620.
kadircet marked 9 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -814,6 +815,514 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68
 
+// Lexes from end of \p FD until it finds a semicolon.
+llvm::Optional getSemiColonForDecl(const FunctionDecl *FD) {

"Lexes" is probably a not very relevant implementation detail.
I'd say this function probably doesn't need a comment, it's clear what it does 
from its name.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:169
+  tooling::Replacements Replacements;
+  std::string Failure;
+  findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) {

NIT: Maybe store `Error` directly?
It would communicate the intent of the variable more clearly.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:175
+// qualifier.
+if (Ref.Qualifier.hasQualifier())
+  return;

NIT: IIUC, this could be simply `if (Ref.Qualifier)`



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:182
+
+// All Targets should be in the same nested name scope, so we can safely
+// chose any one of them.

As discussed before, `Targets` can actually come from different scopes, e.g. 
from ADL.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183
+// All Targets should be in the same nested name scope, so we can safely
+// chose any one of them.
+const NamedDecl *ND = Ref.Targets.front();

It just occurred to me that this is a really bad idea. I **think** the order of 
items in `Targets` is non-deterministic.
Could we instead try to check if all scopes are the same and only qualify in 
that case?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196
+
+std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+if (auto Err = Replacements.add(

You would want to use `ND->printNestedNameSpecifier()` instead to avoid 
printing inline namespaces:
```
namespace std { inline namespace v1 { 
 struct vector {};
}}
```

^-- I believe the current code would print `std::v1::` for `vector` and we want 
`std::`.
Could you add this example to tests too?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:199
+tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
+  Failure = llvm::formatv("Failed to add qualifier: {0}",
+  llvm::fmt_consume(std::move(Err)));

Maybe return a generic error in the end instead of the last error?
Something like `createStringError(..., "Failed to compute qualifiers, see 
errors in the logs for more details")`.

Should be a better UX for anyone who tries to explore what went wrong.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:223
+
+/// Generates Replacements for changing parameter names in \p Dest to be the
+/// same as in \p Source.

NIT: mention that both function parameters and template parameters are updated 
here.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:226
+llvm::Expected
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  const SourceManager  = Dest->getASTContext().getSourceManager();

This does not rename any references to those parameters, right?
E.g.
```
template  void foo(T x);

template  void foo(U x) {}
```

would turn into
```
template  void foo(T x);
```
right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 222892.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Update test helper to take an out parameter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -814,6 +815,500 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+
+  llvm::StringMap EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp",
+  ),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Fix getSemiColon to handle semicolons at the end of file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -815,6 +816,497 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66647#1686021 , @kadircet wrote:

> So currently AST doesn't store any information regarding template parameter 
> locations except the deepest one.
>  Therefore I've changed the availability to discard any methods inside 
> templated classes, since there is no way to
>  validate the template parameter names. Hopefully this should be a rare 
> use-case, and I believe most of the times
>  people have same template paramater names on declaration and definition. Let 
> me know what you think about
>  it.


Thanks! LG, I also think this should be a rare case. The deepest template 
parameters are typically the only ones anyway.

> In addition to that implemented renaming for function parameters and template 
> parameters in case of templated
>  functions.

Thanks, will take a look!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Add renaming of template and function parameters


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -815,6 +816,497 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D66647#1684046 , @ilya-biryukov 
wrote:

> We also need to rename parameters sometimes, right?
>
>   // Sometimes we need to rename parameters.
>   void usages(int decl_param, int);
>  
>   void usages(int def_param, int now_named) {
> llvm::errs() << def_param + now_named;
>   }
>  
>   // And template parameters! (these are even more interesting)
>   template 
>   struct Foo {
> template 
> void usages();
>   };
>   template 
>   template 
>   void Foo::usages() {
> llvm::errs() << L() + R() + NowNamed();
>   }
>


So currently AST doesn't store any information regarding template parameter 
locations except the deepest one.
Therefore I've changed the availability to discard any methods inside templated 
classes, since there is no way to
validate the template parameter names. Hopefully this should be a rare 
use-case, and I believe most of the times
people have same template paramater names on declaration and definition. Let me 
know what you think about
it.

In addition to that implemented renaming for function parameters and template 
parameters in case of templated
functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:77
   //  - if apply() returns an error, returns "fail: "
-  std::string apply(llvm::StringRef MarkedCode) const;
+  std::string apply(llvm::StringRef MarkedCode);
 

kadircet wrote:
> ilya-biryukov wrote:
> > It seems weird that we have this inconsistency between the contents for 
> > current file (passed through the return value) and contents for other files 
> > (pass through the fields).
> > 
> > A few better alternatives that come to mind:
> > 1. add an out parameter, could be optional in case when all changes are in 
> > the main file.
> > 2. this function will fail when there are changes outside main file, but we 
> > can add a new function to return changes in all modified files, e.g. as 
> > `StringMap` or `vector>` (the 
> > latter allows to use standard matchers)
> It is left-out this way since most of the checks only care about a single 
> file, and there are lots of them so changing would definitely require some 
> plumbing.
> 
> I am not sure having the inconsistency between main file and others matters 
> so much though; we have similar discrimination towards non-main files in a 
> bunch of other places in testing infrastructure e.g TestTU.
> 
> As for the suggestions, adding another apply function for multifile case 
> doesn't seem so nice, so I would rather move forward with first one, if you 
> believe this is a strong concern.
> It is left-out this way since most of the checks only care about a single 
> file, and there are lots of them so changing would definitely require some 
> plumbing.
Agree that no matter what we do, we should let the current usages stay the same.

> I am not sure having the inconsistency between main file and others matters 
> so much though; we have similar discrimination towards non-main files in a 
> bunch of other places in testing infrastructure e.g TestTU.

What kind of inconsistency are you referring to? Both main file and extra files 
are fields inside `TestTU`? It outputs `ParsedAST` and it's a return value of 
the `build()` function.
There are no multiple ways to return results.

> As for the suggestions, adding another apply function for multifile case 
> doesn't seem so nice, so I would rather move forward with first one, if you 
> believe this is a strong concern.

Yeah, totally ok with this. I'm not a big fan of out parameters myself and 
prefer putting stuff into the return value instead.
But they're still widely used, so I view this as my preference rather than a 
common convention (sigh...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D66647#1685554 , @ilya-biryukov 
wrote:

> Are we planning to fix this right away or should we keep this indefinitely?
>  If latter, I believe we should think about some heuristics to avoid 
> qualification in the short term instead.
>  If we want to do it right away, that's great! (albeit more complicated, I 
> guess)


That's the end goal, but I am currently working on a patch that will at least 
strip away the common namespaces
of function declaration and symbols used inside.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66647#1685506 , @kadircet wrote:

> I actually have a fixme saying:
>
>   // FIXME: Instead of fully qualifying we should try deducing visible scopes 
> at
>   // target location and generate minimal edits.
>
>
> Elaborating on it by saying "we can start by using the namespaces in 
> targetcontext".


Are we planning to fix this right away or should we keep this indefinitely?
If latter, I believe we should think about some heuristics to avoid 
qualification in the short term instead.
If we want to do it right away, that's great! (albeit more complicated, I guess)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 222102.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -810,6 +811,503 @@
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, 

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D66647#1684012 , @ilya-biryukov 
wrote:

> It's ok to leave this out of the initial change, but could we describe our 
> strategy to tackle this somewhere in the comments - how we want to fix this 
> and when.


I actually have a fixme saying:

  // FIXME: Instead of fully qualifying we should try deducing visible scopes at
  // target location and generate minimal edits.

Elaborating on it by saying "we can start by using the namespaces in 
targetcontext".




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:170
+
+// All Targets should be in the same nested name scope, so we can safely
+// chose any one of them.

ilya-biryukov wrote:
> I don't think it's true in presence of using declarations:
> ```
> namespace ns {
>   void foo(int*);
> }
> namespace other_ns {
>   void foo(char*);
> }
> 
> using ns::foo;
> using other_ns::foo;
> 
> template 
> void usages() {
>   foo(T()); // <-- would we add `ns::foo` or `other_ns::foo`?
> }
> ```
> 
> Not qualifying in this case is probably ok, but adding any of the qualifiers 
> is probably wrong.
> Could you check what we do in this case and either add to tests or put into a 
> list of known issues (e.g. file a bug or at least add a FIXME?)
as discussed offline;
action should not be available in that case and we need underlying 



Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:77
   //  - if apply() returns an error, returns "fail: "
-  std::string apply(llvm::StringRef MarkedCode) const;
+  std::string apply(llvm::StringRef MarkedCode);
 

ilya-biryukov wrote:
> It seems weird that we have this inconsistency between the contents for 
> current file (passed through the return value) and contents for other files 
> (pass through the fields).
> 
> A few better alternatives that come to mind:
> 1. add an out parameter, could be optional in case when all changes are in 
> the main file.
> 2. this function will fail when there are changes outside main file, but we 
> can add a new function to return changes in all modified files, e.g. as 
> `StringMap` or `vector>` (the 
> latter allows to use standard matchers)
It is left-out this way since most of the checks only care about a single file, 
and there are lots of them so changing would definitely require some plumbing.

I am not sure having the inconsistency between main file and others matters so 
much though; we have similar discrimination towards non-main files in a bunch 
of other places in testing infrastructure e.g TestTU.

As for the suggestions, adding another apply function for multifile case 
doesn't seem so nice, so I would rather move forward with first one, if you 
believe this is a strong concern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D66647#1684046 , @ilya-biryukov 
wrote:

> We also need to rename parameters sometimes, right?
>
>   // Sometimes we need to rename parameters.
>   void usages(int decl_param, int);
>  
>   void usages(int def_param, int now_named) {
> llvm::errs() << def_param + now_named;
>   }
>  
>   // And template parameters! (these are even more interesting)
>   template 
>   struct Foo {
> template 
> void usages();
>   };
>   template 
>   template 
>   void Foo::usages() {
> llvm::errs() << L() + R() + NowNamed();
>   }
>


This is tricky, and there are also cases where the declaration doesn't provide 
the name of parameter, e.g. `llvm::json::Value toJSON(const CodeAction &);`. 
Maybe we  can overwrite the declaration signature with the definition's, or 
just disable the tweak on these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

We also need to rename parameters sometimes, right?

  // Sometimes we need to rename parameters.
  void usages(int decl_param, int);
  
  void usages(int def_param, int now_named) {
llvm::errs() << def_param + now_named;
  }
  
  // And template parameters! (these are even more interesting)
  template 
  struct Foo {
template 
void usages();
  };
  template 
  template 
  void Foo::usages() {
llvm::errs() << L() + R() + NowNamed();
  }




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:170
+
+// All Targets should be in the same nested name scope, so we can safely
+// chose any one of them.

I don't think it's true in presence of using declarations:
```
namespace ns {
  void foo(int*);
}
namespace other_ns {
  void foo(char*);
}

using ns::foo;
using other_ns::foo;

template 
void usages() {
  foo(T()); // <-- would we add `ns::foo` or `other_ns::foo`?
}
```

Not qualifying in this case is probably ok, but adding any of the qualifiers is 
probably wrong.
Could you check what we do in this case and either add to tests or put into a 
list of known issues (e.g. file a bug or at least add a FIXME?)



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:176
+  return;
+// Skip members as qualyfying the left side is enough.
+if (ND->isCXXInstanceMember())

NIT: could you add an example of a member expression here?

Note that sometimes there is no left side (it's implicit), but in that case we 
shouldn't qualify either.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:177
+// Skip members as qualyfying the left side is enough.
+if (ND->isCXXInstanceMember())
+  return;

I believe we could also avoid qualifying anything from non-namespace scope.
This follows from the fact that one cannot move stuff from class to namespace 
with using declarations:
```
namespace ns {
class X {
  static void foo();
  void usages();
};
}

using ns::X::foo; // <- not allowed!
void ns::X::usages() {
  foo(); // <-- so no need to qualify this!
}
```
If I'm reading the code correctly, we will get `ns::foo()` in the current 
version.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:227
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+return FTD->getBeginLoc();

Not sure if this covers these two cases (could you please add them to the 
tests?):
```
template 
struct vector {
  void foo();
};

template 
void vector::foo() {}

template 
struct list {
  template 
  void foo();
}

template 
template 
void list::foo() {}
```

Or are we disabled in those cases?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:301
+tooling::Replacements Replacements;
+auto Err = Replacements.add(
+tooling::Replacement(SM, *SemiColon, 1, *QualifiedBody));

This never fails since there are no replacements to conflict with, right?
Maybe we could store a single `tooling::Replacement` here instead? That should 
simplify rest of the code (e.g. no need to `clear()` replacements and re-use 
them for different file)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Currently we add too many qualifiers in some cases, e.g. here's a common 
pattern in clangd:

  // foo.h
  #include "Decl.h"
  namespace clang::clangd { std::string printName(Decl*D); }
  
  // foo.cpp
  #include "foo.h"
  namespace clang::clangd { 
std::string printName(Decl *D) {
  NamedDecl* ND = ...;
} 
  ...
  }

this will result in:

  // foo.h
  namespace clang::clangd {
  std::string printName(Decl*D) {
clang::NamedDecl *ND = ...; // <-- (!) we did not even had any 'using' 
declarations or 'using namespace's 
  }
  }

It's ok to leave this out of the initial change, but could we describe our 
strategy to tackle this somewhere in the comments - how we want to fix this and 
when.




Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:77
   //  - if apply() returns an error, returns "fail: "
-  std::string apply(llvm::StringRef MarkedCode) const;
+  std::string apply(llvm::StringRef MarkedCode);
 

It seems weird that we have this inconsistency between the contents for current 
file (passed through the return value) and contents for other files (pass 
through the fields).

A few better alternatives that come to mind:
1. add an out parameter, could be optional in case when all changes are in the 
main file.
2. this function will fail when there are changes outside main file, but we can 
add a new function to return changes in all modified files, e.g. as 
`StringMap` or `vector>` (the latter 
allows to use standard matchers)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Mark tweak as visible


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -772,6 +773,449 @@
 })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Rebase and update testhelpers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -25,6 +25,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -103,7 +104,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -772,6 +773,449 @@
 })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+using namespace a;
+
+using namespace b;
+using namespace a::b;
+
+using namespace c;
+using namespace b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using b::baz;
+using a::b::baz;
+
+using c::aux;
+using b::c::aux;
+using a::b::c::aux;
+
+namespace d = c;
+namespace d = b::c;
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+using namespace a;
+
+using namespace a::b;
+using namespace a::b;
+
+using namespace a::b::c;
+using namespace a::b::c;
+using namespace a::b::c;
+
+using a::bar;
+
+using a::b::baz;
+using a::b::baz;
+
+using a::b::c::aux;
+using a::b::c::aux;
+using a::b::c::aux;
+
+namespace d = a::b::c;
+namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+  )cpp"),
+R"cpp(
+  void foo()/*Comment -_-*/  {
+class Foo {
+public:
+  void foo();
+  int x;
+  static int y;
+};
+Foo::y = 0;
+
+enum En { Zero, One };
+En x = Zero;
+
+enum class EnClass { Zero, One };
+EnClass y = EnClass::Zero;
+
+template  class Bar {};
+Bar z;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  ;
+
+  void f^oo() {
+using namespace a;
+bar>.bar();
+aux>();
+  }
+  )cpp"),
+R"cpp(
+  namespace a {
+template  class Bar {
+public:
+  void bar();
+};
+template  T bar;
+template  void aux() {}
+  }
+
+  void foo()/*Comment -_-*/  {
+using namespace a;
+a::bar>.bar();
+a::aux>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+return;
+  }
+  )cpp"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  ;
+  };)cpp";
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+return;
+  })cpp"),
+R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("a.h"),
+R"cpp(
+  class Foo {
+void foo()/*Comment -_-*/  {
+return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:158
+  //  i.e: a::Bar> instead of a::Bar>
+  printTemplateArgumentList(OS, TSTL.getTypePtr()->template_arguments(),
+TD->getASTContext().getPrintingPolicy());

Are we trying to replace the whole name?

We can avoid this problem by not qualifying template arguments in the first 
place.
I.e. instead of replacing the whole name, including template arguments:
```
[[baz]] -> [[::ns::baz]]
```
We could simply replace the part before template arguments:
```
[[baz]] -> [[::ns::baz]]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647



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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay.
Herald added a project: clang.
kadircet added a parent revision: D65433: [clangd] DefineInline action 
availability checks.

Initial version of DefineInline action that will fully qualify every
name inside function body. It has some problems while qualyfying dependent
template arguments, see FIXME in the unittests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66647

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
@@ -15,6 +15,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
@@ -75,10 +76,12 @@
 auto T = prepareTweak(ID, Sel);
 if (Available)
   EXPECT_THAT_EXPECTED(T, Succeeded())
-  << "code is " << markRange(Code.code(), Selection);
+  << "code is " << markRange(Code.code(), Selection)
+  << Sel.ASTSelection;
 else
   EXPECT_THAT_EXPECTED(T, Failed())
-  << "code is " << markRange(Code.code(), Selection);
+  << "code is " << markRange(Code.code(), Selection)
+  << Sel.ASTSelection;
   };
   for (auto P : Code.points())
 CheckOver(Range{P, P});
@@ -101,7 +104,9 @@
 std::move(AdditionalFiles));
 }
 
-llvm::Expected apply(StringRef ID, llvm::StringRef Input) {
+llvm::Expected
+apply(StringRef ID, llvm::StringRef Input,
+  const llvm::StringMap  = {}) {
   Annotations Code(Input);
   Range SelectionRng;
   if (Code.points().size() != 0) {
@@ -114,6 +119,9 @@
   TestTU TU;
   TU.Filename = "foo.cpp";
   TU.Code = Code.code();
+  for (auto  : AdditionalFiles) {
+TU.AdditionalFiles[InpFile.first()] = InpFile.second.InitialText;
+  }
 
   ParsedAST AST = TU.build();
   unsigned Begin = cantFail(positionToOffset(Code.code(), SelectionRng.start));
@@ -126,26 +134,44 @@
   return (*T)->apply(S);
 }
 
-llvm::Expected applyEdit(StringRef ID, llvm::StringRef Input) {
-  auto Effect = apply(ID, Input);
+llvm::Expected>
+applyEdit(StringRef ID, llvm::StringRef Input,
+  const llvm::StringMap  = {}) {
+  auto Effect = apply(ID, Input, AdditionalFiles);
   if (!Effect)
 return Effect.takeError();
   if (!Effect->ApplyEdits)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
-  auto Edits = *Effect->ApplyEdits;
-  if (Edits.size() > 1)
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Multi-file edits");
-  Annotations Code(Input);
-  return applyAllReplacements(Code.code(), Edits.begin()->second.Reps);
+  llvm::StringMap Output;
+  for (const auto  : *Effect->ApplyEdits) {
+llvm::StringRef FilePath = It.first();
+FilePath.consume_front(testRoot());
+FilePath.consume_front("/");
+auto InpIt = AdditionalFiles.find(FilePath);
+if (InpIt == AdditionalFiles.end())
+  FilePath = "";
+Annotations Code(InpIt == AdditionalFiles.end()
+ ? Input
+ : llvm::StringRef(InpIt->second.InitialText));
+auto Replaced = applyAllReplacements(Code.code(), It.second.Reps);
+if (!Replaced)
+  return Replaced.takeError();
+Output[FilePath] = *Replaced;
+  }
+  return Output;
 }
 
 void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
-std::string Output) {
-  auto Result = applyEdit(ID, Input);
+std::string Output,
+llvm::StringMap AdditionalFiles = {}) {
+  auto Result = applyEdit(ID, Input, AdditionalFiles);
   ASSERT_TRUE(bool(Result)) << llvm::toString(Result.takeError()) << Input;
-  EXPECT_EQ(Output, std::string(*Result)) << Input;
+  EXPECT_EQ(Output, Result->lookup("")) << Input;
+  for (const auto  : AdditionalFiles) {
+llvm::errs() << "Looking for: " << File.first() << '\n';
+EXPECT_EQ(File.second.ModifiedText, Result->lookup(File.first())) << Input;
+  }
 }
 
 TWEAK_TEST(SwapIfBranches);
@@ -154,7 +180,8 @@
   EXPECT_EQ(apply("^if (true) {return 100;} else {continue;}"),
 "if (true) {continue;} else {return 100;}");
   EXPECT_EQ(apply("^if () {return 100;} else {continue;}"),
-"if () {continue;} else {return 100;}") << "broken condition";
+"if () {continue;} else {return 100;}")
+  << "broken condition";
   EXPECT_AVAILABLE("^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }");