[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f251eece466: [clangd] Define out-of-line availability 
checks (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1807,6 +1807,67 @@
   EXPECT_EQ(apply(Test), Expected) << Test;
 }
 
+TWEAK_TEST(DefineOutline);
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  FileName = "Test.cpp";
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  FileName = "Test.hpp";
+  // Not available unless function name or fully body is selected.
+  EXPECT_UNAVAILABLE(R"cpp(
+// Not a definition
+vo^i[[d^ ^f]]^oo();
+
+[[vo^id ]]foo[[()]] {[[
+  [[(void)(5+3);
+  return;]]
+}]])cpp");
+
+  // Available even if there are no implementation files.
+  EXPECT_AVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  // Not available for out-of-line methods.
+  EXPECT_UNAVAILABLE(R"cpp(
+class Bar {
+  void baz();
+};
+
+[[void [[Bar::[[b^a^z() [[{
+  return;
+})cpp");
+
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+class Bar {
+  [[void [[f^o^o]]() [[{ return; }
+};
+
+void foo();
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  // Not available on defaulted/deleted members.
+  EXPECT_UNAVAILABLE(R"cpp(
+class Foo {
+  Fo^o() = default;
+  F^oo(const Foo&) = delete;
+};)cpp");
+
+  // Not available within templated classes, as it is hard to spell class name
+  // out-of-line in such cases.
+  EXPECT_UNAVAILABLE(R"cpp(
+template  struct Foo { void fo^o(){} };
+})cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/TweakTesting.h
@@ -12,6 +12,7 @@
 #include "TestTU.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -62,6 +63,8 @@
   // testcases.
   std::string Header;
 
+  llvm::StringRef FileName = "TestTU.cpp";
+
   // Extra flags passed to the compilation in apply().
   std::vector ExtraArgs;
 
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -63,12 +63,14 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P6(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
+MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
+   FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
@@ -91,6 +93,7 @@
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
@@ -132,7 +135,7 @@
 
 ::testing::Matcher TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraArgs,
-  ExtraFiles, Index.get());
+  ExtraFiles, Index.get(), FileName);
 }
 
 std::vector TweakTest::expandCases(llvm::StringRef MarkedCode) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -0,0 +1,109 @@
+//===--- DefineOutline.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//

[PATCH] D69266: [clangd] Define out-of-line availability checks

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 60292 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Address comments
- Bail out on templated classes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1809,6 +1809,67 @@
   EXPECT_EQ(apply(Test), Expected) << Test;
 }
 
+TWEAK_TEST(DefineOutline);
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  FileName = "Test.cpp";
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  FileName = "Test.hpp";
+  // Not available unless function name or fully body is selected.
+  EXPECT_UNAVAILABLE(R"cpp(
+// Not a definition
+vo^i[[d^ ^f]]^oo();
+
+[[vo^id ]]foo[[()]] {[[
+  [[(void)(5+3);
+  return;]]
+}]])cpp");
+
+  // Available even if there are no implementation files.
+  EXPECT_AVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  // Not available for out-of-line methods.
+  EXPECT_UNAVAILABLE(R"cpp(
+class Bar {
+  void baz();
+};
+
+[[void [[Bar::[[b^a^z() [[{
+  return;
+})cpp");
+
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+class Bar {
+  [[void [[f^o^o]]() [[{ return; }
+};
+
+void foo();
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  // Not available on defaulted/deleted members.
+  EXPECT_UNAVAILABLE(R"cpp(
+class Foo {
+  Fo^o() = default;
+  F^oo(const Foo&) = delete;
+};)cpp");
+
+  // Not available within templated classes, as it is hard to spell class name
+  // out-of-line in such cases.
+  EXPECT_UNAVAILABLE(R"cpp(
+template  struct Foo { void fo^o(){} };
+})cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/TweakTesting.h
@@ -12,6 +12,7 @@
 #include "TestTU.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -62,6 +63,8 @@
   // testcases.
   std::string Header;
 
+  llvm::StringRef FileName = "TestTU.cpp";
+
   // Extra flags passed to the compilation in apply().
   std::vector ExtraArgs;
 
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -63,12 +63,14 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P6(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
+MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
+   FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
@@ -91,6 +93,7 @@
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
@@ -132,7 +135,7 @@
 
 ::testing::Matcher TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraArgs,
-  ExtraFiles, Index.get());
+  ExtraFiles, Index.get(), FileName);
 }
 
 std::vector TweakTest::expandCases(llvm::StringRef MarkedCode) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -0,0 +1,109 @@
+//===--- DefineOutline.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

still lgtm.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:72
+// definition even if we are inside a source file.
+if (!Sel.AST.getASTContext().getLangOpts().IsHeaderFile)
+  return false;

nit: we have the `isHeaderFile` helper in the `SourceCode.h`, please use it (it 
is more precise).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1549
+
+  // Not available for out-of-line metohds.
+  EXPECT_UNAVAILABLE(R"cpp(

nit: metohds => methods.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1519,6 +1519,62 @@
 using namespace a;
 )cpp");
 }
+
+TWEAK_TEST(DefineOutline);
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  FileName = "Test.cpp";
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  FileName = "Test.hpp";
+  // Not available unless function name or fully body is selected.
+  EXPECT_UNAVAILABLE(R"cpp(
+// Not a definition
+vo^i[[d^ ^f]]^oo();
+
+[[vo^id ]]foo[[()]] {[[
+  [[(void)(5+3);
+  return;]]
+}]])cpp");
+
+  // Available even if there are no implementation files.
+  EXPECT_AVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  // Not available for out-of-line metohds.
+  EXPECT_UNAVAILABLE(R"cpp(
+class Bar {
+  void baz();
+};
+
+[[void [[Bar::[[b^a^z() [[{
+  return;
+})cpp");
+
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+class Bar {
+  [[void [[f^o^o]]() [[{ return; }
+};
+
+void foo();
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  // Not available on defaulted/deleted members.
+  EXPECT_UNAVAILABLE(R"cpp(
+class Foo {
+  Fo^o() = default;
+  F^oo(const Foo&) = delete;
+};)cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/TweakTesting.h
@@ -12,6 +12,7 @@
 #include "TestTU.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -62,6 +63,8 @@
   // testcases.
   std::string Header;
 
+  llvm::StringRef FileName = "TestTU.cpp";
+
   // Extra flags passed to the compilation in apply().
   std::vector ExtraArgs;
 
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -62,12 +62,13 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P5(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index,
+MATCHER_P6(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index, FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.AdditionalFiles = std::move(ExtraFiles);
@@ -89,6 +90,7 @@
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
@@ -125,7 +127,7 @@
 
 ::testing::Matcher TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraFiles,
-  Index.get());
+  Index.get(), FileName);
 }
 
 std::vector TweakTest::expandCases(llvm::StringRef MarkedCode) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -0,0 +1,100 @@
+//===--- DefineOutline.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "HeaderSourceSwitch.h"
+#include "Path.h"
+#include "Selection.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "llvm/ADT/Optional.h"
+#include 

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1548
+
+  ExtraFiles["Test.cpp"] = "";
+  // Basic check for function body and signature.

hokein wrote:
> Is this needed? It is unclear to me why we need it.
yeah not anymore, I was checking for existence of implementation file in the 
first version. but moved it from prepare to apply, forgot to update the test.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1556
+
+[[void [[Bar::[[b^a^z() [[{
+  return;

hokein wrote:
> Sorry for not spotting it earlier.
> 
> Moving out-of-line methods is different than general functions, `void 
> Bar::bar();` is illegal in C++ (C++ requires the out-of-line member 
> declaration must be a definition). I think for this case, we could 1) delete 
> the original method definition 2) disable the tweak. I slightly prefer 2) as 
> out-of-line member definitions are rare in header files. WDYT?
oopsy thanks for spotting this. yeah I agree with 2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1556
+
+[[void [[Bar::[[b^a^z() [[{
+  return;

Sorry for not spotting it earlier.

Moving out-of-line methods is different than general functions, `void 
Bar::bar();` is illegal in C++ (C++ requires the out-of-line member declaration 
must be a definition). I think for this case, we could 1) delete the original 
method definition 2) disable the tweak. I slightly prefer 2) as out-of-line 
member definitions are rare in header files. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:77
+Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+if (!Source || !Source->isThisDeclarationADefinition())
+  return false;

I think `doesThisDeclarationHaveABody` is probably a better choice here? 
`isThisDeclarationADefinition` returns true for defaulted methods like `void 
Ctor() = default;`, we'd need to disable the tweak for this case, could you add 
a test case for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1525
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(

nit: please explicitly spell out the FileName, even the default value is 
`TestTU.cpp`.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1548
+
+  ExtraFiles["Test.cpp"] = "";
+  // Basic check for function body and signature.

Is this needed? It is unclear to me why we need it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:29
+// criteria is met.
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+  if (!SelNode)

hokein wrote:
> nit: looks like we also a similar version in `DefineInline`? would be nice if 
> we could share the implementation. I don't have a good idea where to put it, 
> maybe add a FIXME?
I also had the same concern, but left it here since couldn't find a suitable 
please.
adding a fixme.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:44
+/// Moves definition of a function/method to an appropriate implementation 
file.
+/// If current file is already an implementation file, tries to move the
+/// definition out-of-line.

hokein wrote:
> I'm not sure this is useful in general: saying in .cc we have a single 
> definition `void foo() {}`, after running the code action, we will end up 
> with `void foo(); void foo() {}`.
> 
> If we want to do this, I think we may make it only available for inline class 
> methods.
yes this was implying methods in my head, but forgot to spell it out. updating 
comment.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:75
+
+// Bail out if we are not in a header file.
+// FIXME: We might want to consider moving method definitions below class

hokein wrote:
> The bail-out here seems violating the above comment `/// If current file is 
> already an implementation file, tries to move the definition out-of-line`.  
> Basically, now we only allow moving the function definition to a .cc file.
this was actually representing the state I wanted to arrive, but you are right 
it looks confusing, getting rid of the mention in the class documention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 226045.
kadircet marked 8 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1519,6 +1519,49 @@
 using namespace a;
 )cpp");
 }
+
+TWEAK_TEST(DefineOutline);
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  FileName = "Test.hpp";
+  // Not available unless function name or fully body is selected.
+  EXPECT_UNAVAILABLE(R"cpp(
+// Not a definition
+vo^i[[d^ ^f]]^oo();
+
+[[vo^id ]]foo[[()]] {[[
+  [[(void)(5+3);
+  return;]]
+}]])cpp");
+
+  // Available even if there are no implementation files.
+  EXPECT_AVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  ExtraFiles["Test.cpp"] = "";
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+class Bar {
+  void baz();
+  [[void [[f^o^o]]() [[{ return; }
+};
+
+[[void [[Bar::[[b^a^z() [[{
+  return;
+}
+
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/TweakTesting.h
@@ -12,6 +12,7 @@
 #include "TestTU.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -62,6 +63,8 @@
   // testcases.
   std::string Header;
 
+  llvm::StringRef FileName = "TestTU.cpp";
+
   // Extra flags passed to the compilation in apply().
   std::vector ExtraArgs;
 
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -62,12 +62,13 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P5(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index,
+MATCHER_P6(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index, FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.AdditionalFiles = std::move(ExtraFiles);
@@ -89,6 +90,7 @@
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
@@ -125,7 +127,7 @@
 
 ::testing::Matcher TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraFiles,
-  Index.get());
+  Index.get(), FileName);
 }
 
 std::vector TweakTest::expandCases(llvm::StringRef MarkedCode) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -0,0 +1,99 @@
+//===--- DefineOutline.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "HeaderSourceSwitch.h"
+#include "Path.h"
+#include "Selection.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// Deduces the FunctionDecl from a selection. Requires either the function body
+// or the function decl to be selected. Returns null if none of the above
+// criteria is met.
+// FIXME: This is shared with define inline, 

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the patch looks mostly good. would be interesting to see `apply` implementation.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:29
+// criteria is met.
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+  if (!SelNode)

nit: looks like we also a similar version in `DefineInline`? would be nice if 
we could share the implementation. I don't have a good idea where to put it, 
maybe add a FIXME?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:44
+/// Moves definition of a function/method to an appropriate implementation 
file.
+/// If current file is already an implementation file, tries to move the
+/// definition out-of-line.

I'm not sure this is useful in general: saying in .cc we have a single 
definition `void foo() {}`, after running the code action, we will end up with 
`void foo(); void foo() {}`.

If we want to do this, I think we may make it only available for inline class 
methods.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:75
+
+// Bail out if we are not in a header file.
+// FIXME: We might want to consider moving method definitions below class

The bail-out here seems violating the above comment `/// If current file is 
already an implementation file, tries to move the definition out-of-line`.  
Basically, now we only allow moving the function definition to a .cc file.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:78
+// definition even if we are inside a source file.
+auto Type = driver::types::lookupTypeForExtension(
+llvm::sys::path::extension(FileName).substr(1));

I think we can use `AST.getASTContext().getLangOpts().IsHeaderFile;` to detect 
the header file rather than relying on the filename extension.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1552
+class Bar {
+  void baz();
+};

could you add a testcase where the method is inline? like

```
class Bar {
  void baz2() { return; }
}
``` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69266



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

[PATCH] D69266: [clangd] Define out-of-line availability checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Initial availability checks for performing define out-of-line code
action, which is a refactoring that will help users move function/method
definitions from headers to implementation files.

Proposed implementation only checks whether we have an interesting selection,
namely function name or full function definition/body.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69266

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1519,6 +1519,48 @@
 using namespace a;
 )cpp");
 }
+
+TWEAK_TEST(DefineOutline);
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  FileName = "Test.hpp";
+  // Not available unless function name or fully body is selected.
+  EXPECT_UNAVAILABLE(R"cpp(
+// Not a definition
+vo^i[[d^ ^f]]^oo();
+
+[[vo^id ]]foo[[()]] {[[
+  [[(void)(5+3);
+  return;]]
+}]])cpp");
+
+  // Available even if there are no implementation files.
+  EXPECT_AVAILABLE(R"cpp(
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+
+  ExtraFiles["Test.cpp"] = "";
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+class Bar {
+  void baz();
+};
+
+[[void [[Bar::[[b^a^z() [[{
+  return;
+}
+
+[[void [[f^o^o]]() [[{
+  return;
+})cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/TweakTesting.h
@@ -12,6 +12,7 @@
 #include "TestTU.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
@@ -62,6 +63,8 @@
   // testcases.
   std::string Header;
 
+  llvm::StringRef FileName = "TestTU.cpp";
+
   // Extra flags passed to the compilation in apply().
   std::vector ExtraArgs;
 
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -62,12 +62,13 @@
   cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P5(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index,
+MATCHER_P6(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index, FileName,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.AdditionalFiles = std::move(ExtraFiles);
@@ -89,6 +90,7 @@
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
@@ -125,7 +127,7 @@
 
 ::testing::Matcher TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraFiles,
-  Index.get());
+  Index.get(), FileName);
 }
 
 std::vector TweakTest::expandCases(llvm::StringRef MarkedCode) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -0,0 +1,108 @@
+//===--- DefineOutline.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "HeaderSourceSwitch.h"
+#include "Path.h"
+#include "Selection.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceManager.h"
+#include