[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Thank you. Could you please commit this for me with `Julian Schmidt 
<44101708+5chmi...@users.noreply.github.com>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti marked an inline comment as done.
5chmidti added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+// Allow expressions, but only allow completely selected lambda
+// expressions or unselected lambda expressions that are the parent of
+// the originally selected node, not partially selected lambda

nridge wrote:
> nridge wrote:
> > 5chmidti wrote:
> > > nridge wrote:
> > > > I think it's worth adding to the comment *why* we allow unselected 
> > > > lambda expressions (to allow extracting from capture initializers), as 
> > > > it's not obvious
> > > I changed the previous return of `!isa(Stmt) || 
> > > InsertionPoint->Selected != SelectionTree::Partial;` to a simple `return 
> > > true;`. None of my partial selection tests fail and I can't think of a 
> > > case where the lambda would be partially selected.
> > Hmm, so what actually happens in these testcases?
> > 
> > ```
> >   // lambdas: partially selected
> >   [][[(){}]];
> >   []()[[{}]];
> >   [ [[ ](){}]];
> > ```
> I checked, and in these cases we disallow the extraction 
> [here](https://searchfox.org/llvm/rev/c2bee1ed26a3355d164c92f1eb70ebf88804560d/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp#426-432)
>  before we get to creating an `ExtractionContext`.
> 
> If the testcases are modified as follows:
> 
> ```
> template  void sink(T) {}
> ...
> void f() {
>   // lambdas: partially selected
>   sink([][[(){}]]);
>   sink([]()[[{}]]);
>   sink([ [[ ](){}]]);
> }
> ```
> 
> now they fail, unless the check for partial selection here is restored.
As stated in the update comment, I see no reason why we actually block partial 
selection of lambdas. It's okay to extract from the initializer of a capture 
(one of your comments above):
```
int foo();

void bar() {
  [x = [[foo()]] ]() {};
}
```
The tests in ln 138-143 (titled `lambdas: captures`) check that we don't 
extract the wrong things from lambda captures.
Do you see a way that extracting from a lambda capture or any kind of partial 
selection could be problematic?

To answer the question anyway:
`Hmm, so what actually happens in these testcases?`
The original diff had a sink that I probably removed because the tests would 
succeed even without the sink. However, those tests then tested a different 
condition and never hit the check for partial selection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 556349.
5chmidti added a comment.

Fixup to last revision change:

- do not block the partial selection of lambdas

There is no reason to block the partial selection of lambdas. Other expressions 
can be partially selected as well and still offer an extraction:

  4[[0 +]] 2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -66,6 +66,11 @@
 Code completion
 ^^^
 
+Code actions
+
+
+- The extract variable tweak gained support for extracting lambda expressions to a variable.
+
 Signature help
 ^^
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,7 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,10 +132,80 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas: captures
+  int x = 0;
+  [ [[=]] ](){};
+  [ [[&]] ](){};
+  [ [[x]] ](){};
+  [ [[]] ](){};
+  [y = [[x]] ](){};
+  [ [[y = x]] ](){};
+
+  // lambdas: default args, cannot extract into function-local scope
+  [](int x = [[10]]){};
+  [](auto h = [[ [i = [](){}](){} ]]) {};
+
+  // lambdas: default args
+  // Extracting from capture initializers is usually fine,
+  // but not if the capture itself is nested inside a default argument
+  [](auto h = [i = [[ [](){} ]]](){}) {};
+  [](auto h = [i = [[ 42 ]]](){}) {};
+
+  // lambdas: scope
+  if (int a = 1)
+if ([[ [&](){ return a + 1; } ]]() == 4)
+  a = a + 1;
+
+  for (int c = 0; [[ [&]() { return c < b; } ]](); ++c) {
+  }
+  for (int c = 0; [[ [&]() { return c < b; } () ]]; ++c) {
+  }
+
+  // lambdas: scope with structured binding
+  struct Coordinates {
+int x{};
+int y{};
+  };
+  Coordinates c{};
+  if (const auto [x, y] = c; x > y)
+auto f = [[ [&]() { return x + y; } ]];
+
+  // lambdas: referencing outside variables that block extraction
+  //  in trailing return type or in a decltype used
+  //  by a parameter
+  if (int a = 1)
+if ([[ []() -> decltype(a) { return 1; } ]] () == 4)
+  a = a + 1;
+  if (int a = 1)
+if ([[ [](int x = decltype(a){}) { return 1; } ]] () == 4)
+  a = a + 1;
+  if (int a = 1)
+if ([[ [](decltype(a) x) { return 1; } ]] (42) == 4)
+  a = a + 1;
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
 
+  ExtraArgs = {"-std=c++20"};
+  const char *UnavailableCasesCXX20 = R"cpp(
+template 
+concept Constraint = requires (T t) { true; };
+void foo() {
+  // lambdas: referencing outside variables that block extraction
+  //  in requires clause or defaulted explicit template parameters
+  if (int a = 1)
+if ([[ [](auto b) requires (Constraint && Constraint) { return true; } ]] (a))
+  a = a + 1;
+
+  if (int a = 1)
+if ([[ [](T b) { return true; } ]] (a))
+  a = a + 1;
+}
+  )cpp";
+  EXPECT_UNAVAILABLE(UnavailableCasesCXX20);
+  ExtraArgs.clear();
+
   // vector of pairs of input and output strings
   std::vector> InputOutputs = {
   // extraction from variable declaration/assignment
@@ -282,6 +353,219 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([x = [[40 + 2]] ](){ return 42; });
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = 40 + 2; f([x = placeholder ](){ return 42; });
+}
+)cpp"},
+  {R"cpp(auto foo(int VarA) {
+  return 

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 556348.
5chmidti added a comment.

addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -66,6 +66,11 @@
 Code completion
 ^^^
 
+Code actions
+
+
+- The extract variable tweak gained support for extracting lambda expressions to a variable.
+
 Signature help
 ^^
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,10 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+
+template 
+void sink(T) {}
+
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,10 +135,85 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas: partially selected
+  sink([] [[ (){} ]] );
+  sink([ [[ ]( ]] )[[{}]]);
+  sink([ [[ ](){} ]] );
+
+  // lambdas: captures
+  int x = 0;
+  [ [[=]] ](){};
+  [ [[&]] ](){};
+  [ [[x]] ](){};
+  [ [[]] ](){};
+  [y = [[x]] ](){};
+  [ [[y = x]] ](){};
+
+  // lambdas: default args, cannot extract into function-local scope
+  [](int x = [[10]]){};
+  [](auto h = [[ [i = [](){}](){} ]]) {};
+
+  // lambdas: default args
+  // Extracting from capture initializers is usually fine,
+  // but not if the capture itself is nested inside a default argument
+  [](auto h = [i = [[ [](){} ]]](){}) {};
+  [](auto h = [i = [[ 42 ]]](){}) {};
+
+  // lambdas: scope
+  if (int a = 1)
+if ([[ [&](){ return a + 1; } ]]() == 4)
+  a = a + 1;
+
+  for (int c = 0; [[ [&]() { return c < b; } ]](); ++c) {
+  }
+  for (int c = 0; [[ [&]() { return c < b; } () ]]; ++c) {
+  }
+
+  // lambdas: scope with structured binding
+  struct Coordinates {
+int x{};
+int y{};
+  };
+  Coordinates c{};
+  if (const auto [x, y] = c; x > y)
+auto f = [[ [&]() { return x + y; } ]];
+
+  // lambdas: referencing outside variables that block extraction
+  //  in trailing return type or in a decltype used
+  //  by a parameter
+  if (int a = 1)
+if ([[ []() -> decltype(a) { return 1; } ]] () == 4)
+  a = a + 1;
+  if (int a = 1)
+if ([[ [](int x = decltype(a){}) { return 1; } ]] () == 4)
+  a = a + 1;
+  if (int a = 1)
+if ([[ [](decltype(a) x) { return 1; } ]] (42) == 4)
+  a = a + 1;
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
 
+  ExtraArgs = {"-std=c++20"};
+  const char *UnavailableCasesCXX20 = R"cpp(
+template 
+concept Constraint = requires (T t) { true; };
+void foo() {
+  // lambdas: referencing outside variables that block extraction
+  //  in requires clause or defaulted explicit template parameters
+  if (int a = 1)
+if ([[ [](auto b) requires (Constraint && Constraint) { return true; } ]] (a))
+  a = a + 1;
+
+  if (int a = 1)
+if ([[ [](T b) { return true; } ]] (a))
+  a = a + 1;
+}
+  )cpp";
+  EXPECT_UNAVAILABLE(UnavailableCasesCXX20);
+  ExtraArgs.clear();
+
   // vector of pairs of input and output strings
   std::vector> InputOutputs = {
   // extraction from variable declaration/assignment
@@ -282,6 +361,219 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([x = [[40 + 2]] ](){ return 42; });
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = 40 + 2; f([x = placeholder ](){ return 42; });
+}
+)cpp"},
+  {R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+return [[ [VarA, VarC = 

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti marked an inline comment as done.
5chmidti added a comment.

Ping


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

https://reviews.llvm.org/D141757

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Thanks for your review comments :) . I'll incorporate them and reference your 
comments when I post the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

In D128861#4604193 , @PiotrZSL wrote:

> But for me auto-fixes in this check aren't needed, this simply complicate a 
> check that could be very simple.

You might be right. I will fix the issues I have found and will propose this 
check without the fixes. For completeness, I will mention that I have the 
implementation for them available, and raise your concern about the complexity 
of introducing these fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti abandoned this revision.
5chmidti added a comment.

Because there was no activity on this patch, I am closing it. I should've 
pinged a bit more :) 
With the move to GitHub pull requests soon, pushing this patch further in 
phabricator does not make sense to me. Instead, I could reopen this patch as a 
pull request. There are some problems I have encountered anyway that need 
fixing.
Is there general interest in this check?
The cppcoreguidelines prefix could be changed to something else (e.g. 
modernize), if that is a hindrance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping. Because Phabricator is set to be made read-only on the 1. of October, it 
might be better to not review this patch here and instead open it on GitHub 
instead. Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti marked 4 inline comments as done.
5chmidti added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+// Local variables declared inside of the selected lambda cannot go out of
+// scope. The DeclRefExprs that are important are the variables captured 
and

nridge wrote:
> 5chmidti wrote:
> > nridge wrote:
> > > Looking at 
> > > [RecursiveASTVisitor::TraverseLambdaExpr](https://searchfox.org/llvm/rev/094539cfcb46f55824402565e5c18580df689a67/clang/include/clang/AST/RecursiveASTVisitor.h#2652-2691),
> > >  there are a few things it traverses in addition to the lambda's 
> > > parameters and body (which we are saying are ok to skip) and the lambda's 
> > > captures (which we are traversing).
> > > 
> > > For example, the lambda may have an explicit result type which references 
> > > a variable in scope:
> > > 
> > > ```
> > > int main() {
> > >   if (int a = 1)
> > > if ([[ []() -> decltype(a){ return 1; } ]] () == 4)
> > >   a = a + 1;
> > > }
> > > 
> > > ```
> > > 
> > > Here, extracting the lambda succeeds, and the reference to `a` in 
> > > `decltype(a)` becomes dangling.
> > I'll update the diff when I've implemented this. A requires clause may 
> > reference a variable like `a` as well.
> > A requires clause may reference a variable like `a` as well.
> 
> Technically, an explicit template parameter list can also reference a local 
> variable via e.g. a default argument for a non-type parameter.
> 
> I appreciate that we're getting into increasingly obscure edge cases here, so 
> please feel free to use your judgment and draw a line somewhere; the 
> refactoring introducing a compiler error when invoked on some 
> obscure/unlikely piece of code is not that big of a deal.
I have added support for attributes, trailing return-type decls and explicit 
template parameters to the visitor. I think that is every edge case covered.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+// Allow expressions, but only allow completely selected lambda
+// expressions or unselected lambda expressions that are the parent of
+// the originally selected node, not partially selected lambda

nridge wrote:
> I think it's worth adding to the comment *why* we allow unselected lambda 
> expressions (to allow extracting from capture initializers), as it's not 
> obvious
I changed the previous return of `!isa(Stmt) || 
InsertionPoint->Selected != SelectionTree::Partial;` to a simple `return 
true;`. None of my partial selection tests fail and I can't think of a case 
where the lambda would be partially selected.



Comment at: 
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:149
+  [](int x = [[10]]){};
+  [](auto h = [i = [[ [](){} ]]](){}) {};
+  [](auto h = [[ [i = [](){}](){} ]]) {};

nridge wrote:
> It's easy to overlook the purpose of this line amidst all the brackets, I 
> would suggest adding a comment like:
> 
> ```
> // Extracting from a capture initializer is usually fine, but not if
> // the capture itself is nested inside a default argument
> ```
I added some comments like the one for this example to indicate why an 
extraction is expected to be unavailable. The same goes for the tests just 
above (ln134-189).


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

https://reviews.llvm.org/D141757

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 552065.
5chmidti added a comment.

Addressed comments:

- added explicit template parameters, trailing return-type declarations and 
attributes to the visitor that finds DeclRefExprs
- added a few more comments about why an extraction is expected to be 
unavailable


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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -66,6 +66,11 @@
 Code completion
 ^^^
 
+Code actions
+
+
+- The extract variable tweak gained support for extracting lambda expressions to a variable.
+
 Signature help
 ^^
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -131,10 +131,85 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas: partially selected
+  [][[(){}]];
+  []()[[{}]];
+  [ [[ ](){}]];
+
+  // lambdas: captures
+  int x = 0;
+  [ [[=]] ](){};
+  [ [[&]] ](){};
+  [ [[x]] ](){};
+  [ [[]] ](){};
+  [y = [[x]] ](){};
+  [ [[y = x]] ](){};
+
+  // lambdas: default args, cannot extract into function-local scope
+  [](int x = [[10]]){};
+  [](auto h = [[ [i = [](){}](){} ]]) {};
+
+  // lambdas: default args
+  // Extracting from capture initializers is usually fine,
+  // but not if the capture itself is nested inside a default argument
+  [](auto h = [i = [[ [](){} ]]](){}) {};
+  [](auto h = [i = [[ 42 ]]](){}) {};
+
+  // lambdas: scope
+  if (int a = 1)
+if ([[ [&](){ return a + 1; } ]]() == 4)
+  a = a + 1;
+
+  for (int c = 0; [[ [&]() { return c < b; } ]](); ++c) {
+  }
+  for (int c = 0; [[ [&]() { return c < b; } () ]]; ++c) {
+  }
+
+  // lambdas: scope with structured binding
+  struct Coordinates {
+int x{};
+int y{};
+  };
+  Coordinates c{};
+  if (const auto [x, y] = c; x > y)
+auto f = [[ [&]() { return x + y; } ]];
+
+  // lambdas: referencing outside variables that block extraction
+  //  in trailing return type or in a decltype used
+  //  by a parameter
+  if (int a = 1)
+if ([[ []() -> decltype(a) { return 1; } ]] () == 4)
+  a = a + 1;
+  if (int a = 1)
+if ([[ [](int x = decltype(a){}) { return 1; } ]] () == 4)
+  a = a + 1;
+  if (int a = 1)
+if ([[ [](decltype(a) x) { return 1; } ]] (42) == 4)
+  a = a + 1;
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
 
+  ExtraArgs = {"-std=c++20"};
+  const char *UnavailableCasesCXX20 = R"cpp(
+template 
+concept Constraint = requires (T t) { true; };
+void foo() {
+  // lambdas: referencing outside variables that block extraction
+  //  in requires clause or defaulted explicit template parameters
+  if (int a = 1)
+if ([[ [](auto b) requires (Constraint && Constraint) { return true; } ]] (a))
+  a = a + 1;
+
+  if (int a = 1)
+if ([[ [](T b) { return true; } ]] (a))
+  a = a + 1;
+}
+  )cpp";
+  EXPECT_UNAVAILABLE(UnavailableCasesCXX20);
+  ExtraArgs.clear();
+
   // vector of pairs of input and output strings
   std::vector> InputOutputs = {
   // extraction from variable declaration/assignment
@@ -282,6 +357,219 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([x = [[40 + 2]] ](){ return 42; });
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = 40 + 2; f([x = placeholder ](){ return 42; });
+}
+)cpp"},
+  {R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+return [[ [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }]];
+  

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti marked an inline comment as done.
5chmidti added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+// Local variables declared inside of the selected lambda cannot go out of
+// scope. The DeclRefExprs that are important are the variables captured 
and

nridge wrote:
> Looking at 
> [RecursiveASTVisitor::TraverseLambdaExpr](https://searchfox.org/llvm/rev/094539cfcb46f55824402565e5c18580df689a67/clang/include/clang/AST/RecursiveASTVisitor.h#2652-2691),
>  there are a few things it traverses in addition to the lambda's parameters 
> and body (which we are saying are ok to skip) and the lambda's captures 
> (which we are traversing).
> 
> For example, the lambda may have an explicit result type which references a 
> variable in scope:
> 
> ```
> int main() {
>   if (int a = 1)
> if ([[ []() -> decltype(a){ return 1; } ]] () == 4)
>   a = a + 1;
> }
> 
> ```
> 
> Here, extracting the lambda succeeds, and the reference to `a` in 
> `decltype(a)` becomes dangling.
I'll update the diff when I've implemented this. A requires clause may 
reference a variable like `a` as well.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:182-184
+if (InsertionPoint->Parent->ASTNode.get() != nullptr) {
+  return false;
+}

nridge wrote:
> 5chmidti wrote:
> > This is supposed to stop the following invalid code from happening:
> > ```
> > void foo() {
> >   int placeholder = 42;
> >   [](int x = placeholder {};
> >   extern void bar(int x = placeholder);
> > }
> > ```
> > 
> > clangd does not seem to support extracting from the initializers of 
> > defaulted parameters, should I keep the condition as is, or should I do 
> > something different here? It is legal to have default arguments in global 
> > scope (examples below).
> > 
> > The following insertions could exist (out of scope for this patch):
> > ```
> > int placeholder = 42;
> > void foo() {
> >   [](int x = placeholder {};
> >   extern void bar(int x = placeholder);
> > }
> > ```
> > ```
> > struct X {
> >   static inline int placeholder = 42;
> >   void foo(int x = placeholder) {}
> > };
> > ```
> > 
> > Either way, I'll have to adjust the comment because this is not just to 
> > stop default parameter initializers in lambdas from being extracted to a 
> > local scope.
> I'm having trouble following this comment. Can you give the testcase that 
> would produce this invalid code?
I provided test cases in lines 147-150 in ExtractVariableTests.cpp. For the 
example given in my comment, the test case would be:
Pre:
```
void foo() {
  [](int x = [[42]]) {};
}
```
(erroneous) Post:
```
void foo() {
  int placeholder = 42;
  [](int x = placeholder) {};
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D157773: [clang-tidy] fix None tmpdir when exporting fixes in run-clang-tidy

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Thanks. I don't have commit access, could somebody please commit this for me 
with `Julian Schmidt <44101708+5chmi...@users.noreply.github.com>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157773

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

[PATCH] D157773: [clang-tidy] fix None tmpdir when exporting fixes in run-clang-tidy

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti created this revision.
5chmidti added a project: clang-tools-extra.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
5chmidti requested review of this revision.
Herald added a subscriber: cfe-commits.

Differential https://reviews.llvm.org/D145477 removed the check for `(yaml and 
args.export_fixes)` in line 303 to skip looking for the 
`clang-apply-replacements` binary. However, the `tmpdir` variable was set in 
this true branch when exporting fixes and therefore is `None` when invoking 
run-clang-tidy with `run-clang-tidy -p . -export-fixes fixes.yaml`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157773

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -389,6 +389,8 @@
 clang_apply_replacements_binary = find_binary(
 args.clang_apply_replacements_binary, "clang-apply-replacements", 
build_path
 )
+
+if args.fix or (yaml and args.export_fixes):
 tmpdir = tempfile.mkdtemp()
 
 try:


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -389,6 +389,8 @@
 clang_apply_replacements_binary = find_binary(
 args.clang_apply_replacements_binary, "clang-apply-replacements", build_path
 )
+
+if args.fix or (yaml and args.export_fixes):
 tmpdir = tempfile.mkdtemp()
 
 try:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti requested review of this revision.
5chmidti added a comment.

Never mind, the problem is unrelated to this patch. Filed clangd issue #1710 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti planned changes to this revision.
5chmidti added a comment.

I found a new issue. I'll probably fix it by next weekend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 538289.
5chmidti added a comment.

Ping, and:

- removed mention of captures from the condition about blocking extraction 
because that is allowed
- removed `(of a lambda)` from the following comment because it is not just 
about defaulted parameters of lambdas, but of all functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -66,6 +66,11 @@
 Code completion
 ^^^
 
+Code actions
+
+
+- The extract variable tweak gained support for extracting lambda expressions to a variable.
+
 Signature help
 ^^
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -131,7 +131,43 @@
   goto label;
   label:
 a = [[1]];
-}
+
+  // lambdas
+  [][[(){}]];
+
+  // lambdas: captures
+  int x = 0;
+  [ [[=]] ](){};
+  [ [[&]] ](){};
+  [ [[x]] ](){};
+  [ [[] ]](){};
+  [y = [[x]] ](){};
+  [ [[y = x]] ](){};
+
+  // lambdas: default args
+  [](int x = [[10]]){};
+  [](auto h = [i = [[ [](){} ]]](){}) {};
+  [](auto h = [[ [i = [](){}](){} ]]) {};
+
+  // lambdas: scope
+  if (int a = 1)
+if ([[ [&](){ return a + 1; } ]]() == 4)
+  a = a + 1;
+
+  for (int c = 0; [[ [&]() { return c < b; } ]](); ++c) {
+  }
+  for (int c = 0; [[ [&]() { return c < b; } () ]]; ++c) {
+  }
+
+  // lambdas: scope with structured binding
+  struct Coordinates {
+int x{};
+int y{};
+  };
+  Coordinates c{};
+  if (const auto [x, y] = c; x > y)
+auto f = [[ [&]() { return x + y; } ]];
+  }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
 
@@ -282,6 +318,209 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+return [[ [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }]];
+  };
+}
+)cpp",
+   R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+auto placeholder = [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }; return  placeholder;
+  };
+}
+)cpp"},
+  {R"cpp(template  void f(T) {}
+void f2(int var) {
+  f([[ [](){ auto internal_val = 42; return var + internal_val; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2(int var) {
+  auto placeholder = [](){ auto internal_val = 42; return var + internal_val; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(template  void f(T) { }
+struct A {
+void f2(int& var) {
+auto local_var = 42;
+f([[ [, _var, this]() {
+auto internal_val = 42;
+return var + local_var + internal_val + member;
+}]]);
+}
+
+int member = 42;
+};
+)cpp",
+   R"cpp(template  void f(T) { }
+struct A {
+void f2(int& var) {
+auto local_var = 42;
+auto placeholder = [, _var, this]() {
+auto internal_val = 42;
+return var + local_var + internal_val + member;
+}; f( placeholder);
+}
+
+int member = 42;
+};
+)cpp"},
+  {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
+   R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x =  placeholder; })cpp"},
+  {R"cpp(
+

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 538286.
5chmidti added a comment.

Ping, and:

- change find_if of post-use detection to range-for with a filtered range
- make `renderHoistedCall` a private member of `NewFunction`
- remove the `Render` lambda in `renderHoistSet` in favor of putting the body 
into the loop, improving readability imo.
- swapped order in tertiary operator to remove the `!` in the condition for 
better readability
- add abort condtition if any of the `NamedDecl`s to be hoisted is a 
`TypeDecl`. The `HoistedSet` can contain `TypeDecl`s, which block the 
extraction because types cannot be hoisted as easily as variables. Hoisting 
types could be done by moving the declaration into the scope that the functions 
are declared in (global, namespace or records). I could take a look at this if 
there is interest, but I think that should be its own diff.
- add tests about full and partial selection of type aliases and classes

Open questions:

- I think that ``/`` should be included if a `tuple` or `pair` 
is used, but couldn't figure out a clean way to include the headers. It looks 
like the way to go would be through `IncludeCleaner`s `insert` function, but 
for that I woul dneed to construct the `IncludeInserter`. I don't think I can 
get the `FormatStyle` or `BuildDir` from inside of a tweak. Does anyone have an 
idea or is this a non-issue?
- The tweak is unavailable if a tuple or pair is required for the hoisting, but 
no auto return-type deduction is available. This is the case because I 
implemented the return type of the hoisted variables to use `tuple` or `pair` 
if two or more variables are returned, but to keep the return type short, I set 
the return type in these cases to `auto`. Should the return type be fully 
spelled out? Should it be spelled out always or should there be a config option 
(seems a bit overkill)?
- I'm not a fan of the `const&` `HoistSet` member that I used. Should I change 
this to a pointer (its always non-null)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,9 @@
 Miscellaneous
 ^
 
+- The extract function tweak gained support for hoisting, i.e. returning decls declared
+  inside the selection that are used outside of the selection.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -30,8 +30,9 @@
   EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
   // Partial statements aren't extracted.
   EXPECT_THAT(apply("int [[x = 0]];"), "unavailable");
-  // FIXME: Support hoisting.
-  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), "unavailable");
+
+  // Extract regions that require hoisting
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), HasSubstr("extracted"));
 
   // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
   // lead to break being included in the extraction zone.
@@ -192,6 +193,310 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, Hoisting) {
+  ExtraArgs.emplace_back("-std=c++17");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  std::string HoistingOutput = R"cpp(
+auto extracted(int ) {
+int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;
+return std::tuple{x, y, z};
+}
+int foo() {
+  int a = 3;
+  auto [x, y, z] = extracted(a);
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput), HoistingOutput);
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a{};
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 1;
+return b;
+}
+int foo() {
+  int a{};
+  auto b = extracted(a);
+  return b;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput2), HoistingOutput2);
+
+  std::string HoistingInput3 = R"cpp(
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+[[a = 123;
+return a + b;]]
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  std::string HoistingOutput3 = R"cpp(
+int extracted(int , int ) {
+a = 123;
+return a + b;
+}
+int foo(int 

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:178
+  // Allow all expressions except partial LambdaExpr selections since we
+  // don't want to extract from the captures/default arguments of a lambda
+  if (isa(Stmt)) {

Noticed that this can be removed because extracting from captures is allowed. 
Will change in the next update



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:182-184
+if (InsertionPoint->Parent->ASTNode.get() != nullptr) {
+  return false;
+}

This is supposed to stop the following invalid code from happening:
```
void foo() {
  int placeholder = 42;
  [](int x = placeholder {};
  extern void bar(int x = placeholder);
}
```

clangd does not seem to support extracting from the initializers of defaulted 
parameters, should I keep the condition as is, or should I do something 
different here? It is legal to have default arguments in global scope (examples 
below).

The following insertions could exist (out of scope for this patch):
```
int placeholder = 42;
void foo() {
  [](int x = placeholder {};
  extern void bar(int x = placeholder);
}
```
```
struct X {
  static inline int placeholder = 42;
  void foo(int x = placeholder) {}
};
```

Either way, I'll have to adjust the comment because this is not just to stop 
default parameter initializers in lambdas from being extracted to a local scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D141757: [clangd] allow extracting to variable for complete lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 518159.
5chmidti added a comment.

I took a little break, but here are the changes/fixes:

- moved the logic for variables referenced in captures into the visitor
  - short circuiting the `TraverseLambdaExpression` and using 
`TraverseLambdaCapture` to handle variable captures and the initialization fo 
init-captures
  - fixes both problems mentioned by nridge
- fix for immediately invoked lambda expressions
  - the selection of an IIL would mark the call operator as a referenced decl 
(fixed in `VisitDeclRefExpr`)
- fix condition in `CanExtractOutside` to allow an unselected parent that is a 
lambda expression
  - allows for the extraction of an initializer for init-capture variables
- block default arguments from prarameters of a lambda from being extracted to 
a function-local scope (fixed in `CanExtractOutside`)
- added more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -66,6 +66,11 @@
 Code completion
 ^^^
 
+Code actions
+
+
+- The extract variable tweak gained support for extracting lambda expressions to a variable.
+
 Signature help
 ^^
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -131,7 +131,43 @@
   goto label;
   label:
 a = [[1]];
-}
+
+  // lambdas
+  [][[(){}]];
+
+  // lambdas: captures
+  int x = 0;
+  [ [[=]] ](){};
+  [ [[&]] ](){};
+  [ [[x]] ](){};
+  [ [[] ]](){};
+  [y = [[x]] ](){};
+  [ [[y = x]] ](){};
+
+  // lambdas: default args
+  [](int x = [[10]]){};
+  [](auto h = [i = [[ [](){} ]]](){}) {};
+  [](auto h = [[ [i = [](){}](){} ]]) {};
+
+  // lambdas: scope
+  if (int a = 1)
+if ([[ [&](){ return a + 1; } ]]() == 4)
+  a = a + 1;
+
+  for (int c = 0; [[ [&]() { return c < b; } ]](); ++c) {
+  }
+  for (int c = 0; [[ [&]() { return c < b; } () ]]; ++c) {
+  }
+
+  // lambdas: scope with structured binding
+  struct Coordinates {
+int x{};
+int y{};
+  };
+  Coordinates c{};
+  if (const auto [x, y] = c; x > y)
+auto f = [[ [&]() { return x + y; } ]];
+  }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
 
@@ -282,6 +318,209 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+return [[ [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }]];
+  };
+}
+)cpp",
+   R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+auto placeholder = [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }; return  placeholder;
+  };
+}
+)cpp"},
+  {R"cpp(template  void f(T) {}
+void f2(int var) {
+  f([[ [](){ auto internal_val = 42; return var + internal_val; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2(int var) {
+  auto placeholder = [](){ auto internal_val = 42; return var + internal_val; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(template  void f(T) { }
+struct A {
+void f2(int& var) {
+auto local_var = 42;
+f([[ [, _var, this]() {
+auto internal_val = 42;
+return var + local_var + internal_val + member;
+}]]);
+}
+
+int member = 42;
+};
+)cpp",
+   R"cpp(template  void f(T) { }
+struct A {
+void f2(int& var) {
+   

[PATCH] D141757: [clangd] allow extracting to variable for complete lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

You're right, I'll incorporate the logic for lambdas into `FindDeclRefsVisitor` 
and change the docs/comments/commit message to reflect this. I will try to 
think of more edge cases to test as well. Though I'll be busy until mid march 
with uni, so there will be no new revisions until then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

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

[PATCH] D141757: [clangd] allow extracting to variable for complete lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 491319.
5chmidti added a comment.
Herald added a subscriber: ChuanqiXu.

1. addressed comments

2 bugfix (sorry, should've said something)
Local variables inside the lambda were previously added to the ReferencedDecls 
vector and would block the action inside of exprIsValidOutside (declarations 
are inside of the lambda).
Now only consider DeclRefExprs in the lambda captures.

3. I don't have commit access, someone else would need to commit this.

I don't know if you want to delay this patch due to the bugfix or not, either 
way is fine with me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -66,6 +66,11 @@
 Code completion
 ^^^
 
+Code actions
+
+
+- The extract variable tweak gained support for extracting complete lambda expressions to a variable.
+
 Signature help
 ^^
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,7 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+template  void callable_sink(T) {}
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,6 +132,21 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas
+  callable_sink([][[(){}]]);
+
+  // captures
+  int x = 0;
+  callable_sink([ [[=]] ](){});
+  callable_sink([ [[&]] ](){});
+  callable_sink([ [[x]] ](){});
+  callable_sink([ [[] ]](){});
+  callable_sink([y = [[x]] ](){});
+  callable_sink([ [[y = x]] ](){});
+
+  // default args
+  callable_sink([](int x = [[10]]){});
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
@@ -282,6 +298,67 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // Complete lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+return [[ [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }]];
+  };
+}
+)cpp",
+   R"cpp(auto foo(int VarA) {
+  return [VarA]() {
+auto placeholder = [VarA, VarC = 42 + VarA](int VarB) { return VarA + VarB + VarC; }; return  placeholder;
+  };
+}
+)cpp"},
+  {R"cpp(template  void f(T) {}
+void f2(int var) {
+  f([[ [](){ auto internal_val = 42; return var + internal_val; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2(int var) {
+  auto placeholder = [](){ auto internal_val = 42; return var + internal_val; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(template  void f(T) { }
+struct A {
+void f2(int& var) {
+auto local_var = 42;
+f([[ [, _var, this]() {
+auto internal_val = 42;
+return var + local_var + internal_val + member;
+}]]);
+}
+
+int member = 42;
+};
+)cpp",
+   R"cpp(template  void f(T) { }
+struct A {
+void f2(int& var) {
+auto local_var = 42;
+auto placeholder = [, _var, this]() {
+auto internal_val = 42;
+return var + local_var + internal_val + member;
+}; f( placeholder);
+}
+
+int member = 42;
+};
+)cpp"},
+  {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
+   R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x =  placeholder; })cpp"},
   // Don't try to analyze across macro 

[PATCH] D141760: [clangd] fix extract function tweak signature spelling out lambda types

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti abandoned this revision.
5chmidti added a comment.

Nvmd, `Wrapped` won't work. I'll look into this some more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141760

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

[PATCH] D141760: [clangd] fix extract function tweak signature spelling out lambda types

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti created this revision.
5chmidti added reviewers: sammccall, nridge.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
5chmidti requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Fixes the signature of the extracted function when the signature contains a 
lambda.
Previously spelled out the type (i.e. loc, ...) of the lambda, now specifies 
auto.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141760

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst


Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,8 @@
 Miscellaneous
 ^
 
+- Fixed extracted function signatures spelling out lambda types.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -190,6 +190,32 @@
 }]]
   )cpp";
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
+
+  std::string LambdaInput = R"cpp(
+auto bar() {
+  int x = 10;
+  auto lambda = [x]() { return x; };
+  [[int y = 42;
+  int z = lambda();]]
+
+  return lambda();
+}
+  )cpp";
+
+  std::string LambdaOutput = R"cpp(
+void extracted(auto ) {
+int y = 42;
+  int z = lambda();
+}
+auto bar() {
+  int x = 10;
+  auto lambda = [x]() { return x; };
+  extracted(lambda);
+
+  return lambda();
+}
+  )cpp";
+  EXPECT_EQ(apply(LambdaInput), LambdaOutput);
 }
 
 TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -430,6 +430,11 @@
   PrintCB PCB();
   PP.Callbacks = 
 
+  if (const auto *const RDecl = QT->getAsCXXRecordDecl();
+  RDecl && RDecl->isLambda()) {
+return "auto";
+  }
+
   QT.print(OS, PP, Placeholder);
   return OS.str();
 }


Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,8 @@
 Miscellaneous
 ^
 
+- Fixed extracted function signatures spelling out lambda types.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -190,6 +190,32 @@
 }]]
   )cpp";
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
+
+  std::string LambdaInput = R"cpp(
+auto bar() {
+  int x = 10;
+  auto lambda = [x]() { return x; };
+  [[int y = 42;
+  int z = lambda();]]
+
+  return lambda();
+}
+  )cpp";
+
+  std::string LambdaOutput = R"cpp(
+void extracted(auto ) {
+int y = 42;
+  int z = lambda();
+}
+auto bar() {
+  int x = 10;
+  auto lambda = [x]() { return x; };
+  extracted(lambda);
+
+  return lambda();
+}
+  )cpp";
+  EXPECT_EQ(apply(LambdaInput), LambdaOutput);
 }
 
 TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -430,6 +430,11 @@
   PrintCB PCB();
   PP.Callbacks = 
 
+  if (const auto *const RDecl = QT->getAsCXXRecordDecl();
+  RDecl && RDecl->isLambda()) {
+return "auto";
+  }
+
   QT.print(OS, PP, Placeholder);
   return OS.str();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D141757: [clangd] allow extracting to variable for complete lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 489232.
5chmidti added a comment.

Add change to release notes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst


Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,8 @@
 Miscellaneous
 ^
 
+- The extract variable tweak gained support for extracting complete lambda 
expressions to a variable.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,7 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+template  void callable_sink(T) {}
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,6 +132,21 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas
+  callable_sink([][[(){}]]);
+
+  // captures
+  int x = 0;
+  callable_sink([ [[=]] ](){});
+  callable_sink([ [[&]] ](){});
+  callable_sink([ [[x]] ](){});
+  callable_sink([ [[] ]](){});
+  callable_sink([y = [[x]] ](){});
+  callable_sink([ [[y = x]] ](){});
+
+  // default args
+  callable_sink([](int x = [[10]]){});
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
@@ -282,6 +298,19 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + 
placeholder + S(5);
  })cpp"},
+  // Complete lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
+   R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x =  
placeholder; })cpp"},
   // Don't try to analyze across macro boundaries
   // FIXME: it'd be nice to do this someday (in a safe way)
   {R"cpp(#define ECHO(X) X
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -152,10 +152,11 @@
   auto CanExtractOutside =
   [](const SelectionTree::Node *InsertionPoint) -> bool {
 if (const clang::Stmt *Stmt = InsertionPoint->ASTNode.get()) {
-  // Allow all expressions except LambdaExpr since we don't want to extract
-  // from the captures/default arguments of a lambda
+  // Allow all expressions except partial LambdaExpr selections since we
+  // don't want to extract from the captures/default arguments of a lambda
   if (isa(Stmt))
-return !isa(Stmt);
+return !(isa(Stmt) &&
+ InsertionPoint->Selected != SelectionTree::Complete);
   // We don't yet allow extraction from switch/case stmt as we would need 
to
   // jump over the switch stmt even if there is a CompoundStmt inside the
   // switch. And there are other Stmts which we don't care about (e.g.


Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,8 @@
 Miscellaneous
 ^
 
+- The extract variable tweak gained support for extracting complete lambda expressions to a variable.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,7 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+template  void callable_sink(T) {}
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,6 +132,21 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas
+  callable_sink([][[(){}]]);
+
+  // captures
+  int x = 0;
+  callable_sink([ [[=]] ](){});
+  

[PATCH] D141757: [clangd] allow extracting to variable for complete lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti created this revision.
5chmidti added reviewers: sammccall, nridge.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
5chmidti requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Support for extracting complete lambda expressions, e.g. extracting a lambda 
from a callexpr (e.g. algorithms/ranges) to a named variable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141757

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp


Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,7 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+template  void callable_sink(T) {}
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,6 +132,21 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas
+  callable_sink([][[(){}]]);
+
+  // captures
+  int x = 0;
+  callable_sink([ [[=]] ](){});
+  callable_sink([ [[&]] ](){});
+  callable_sink([ [[x]] ](){});
+  callable_sink([ [[] ]](){});
+  callable_sink([y = [[x]] ](){});
+  callable_sink([ [[y = x]] ](){});
+
+  // default args
+  callable_sink([](int x = [[10]]){});
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
@@ -282,6 +298,19 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + 
placeholder + S(5);
  })cpp"},
+  // Complete lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ [](){ return 42; }]]);
+}
+)cpp",
+   R"cpp(template  void f(T) {}
+void f2() {
+  auto placeholder = [](){ return 42; }; f( placeholder);
+}
+)cpp"},
+  {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
+   R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x =  
placeholder; })cpp"},
   // Don't try to analyze across macro boundaries
   // FIXME: it'd be nice to do this someday (in a safe way)
   {R"cpp(#define ECHO(X) X
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -152,10 +152,11 @@
   auto CanExtractOutside =
   [](const SelectionTree::Node *InsertionPoint) -> bool {
 if (const clang::Stmt *Stmt = InsertionPoint->ASTNode.get()) {
-  // Allow all expressions except LambdaExpr since we don't want to extract
-  // from the captures/default arguments of a lambda
+  // Allow all expressions except partial LambdaExpr selections since we
+  // don't want to extract from the captures/default arguments of a lambda
   if (isa(Stmt))
-return !isa(Stmt);
+return !(isa(Stmt) &&
+ InsertionPoint->Selected != SelectionTree::Complete);
   // We don't yet allow extraction from switch/case stmt as we would need 
to
   // jump over the switch stmt even if there is a CompoundStmt inside the
   // switch. And there are other Stmts which we don't care about (e.g.


Index: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -98,6 +98,7 @@
   return [[t]].bar([[t]].z);
 }
 void v() { return; }
+template  void callable_sink(T) {}
 // function default argument
 void f(int b = [[1]]) {
   // empty selection
@@ -131,6 +132,21 @@
   goto label;
   label:
 a = [[1]];
+
+  // lambdas
+  callable_sink([][[(){}]]);
+
+  // captures
+  int x = 0;
+  callable_sink([ [[=]] ](){});
+  callable_sink([ [[&]] ](){});
+  callable_sink([ [[x]] ](){});
+  callable_sink([ [[] ]](){});
+  callable_sink([y = [[x]] ](){});
+  callable_sink([ [[y = x]] ](){});
+
+  // default args
+  callable_sink([](int x = [[10]]){});
 }
   )cpp";
   EXPECT_UNAVAILABLE(UnavailableCases);
@@ -282,6 +298,19 @@
  void f() {
auto placeholder = S(2) + S(3) + S(4); S x = S(1) + placeholder + S(5);
  })cpp"},
+  // Complete lambda expressions
+  {R"cpp(template  void f(T) {}
+void f2() {
+  f([[ 

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

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

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 477204.
5chmidti added a comment.

Fix windows build by setting the c++ standard for the hoisting tests explicitly 
to c++17.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,9 @@
 Miscellaneous
 ^
 
+- The extract function tweak gained support for hoisting, i.e. returning decls declared
+  inside the selection that are used outside of the selection.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -30,8 +30,9 @@
   EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
   // Partial statements aren't extracted.
   EXPECT_THAT(apply("int [[x = 0]];"), "unavailable");
-  // FIXME: Support hoisting.
-  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), "unavailable");
+
+  // Extract regions that require hoisting
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), HasSubstr("extracted"));
 
   // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
   // lead to break being included in the extraction zone.
@@ -192,6 +193,203 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, Hoisting) {
+  ExtraArgs.emplace_back("-std=c++17");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  std::string HoistingOutput = R"cpp(
+auto extracted(int ) {
+int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;
+return std::tuple{x, y, z};
+}
+int foo() {
+  int a = 3;
+  auto [x, y, z] = extracted(a);
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput), HoistingOutput);
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a{};
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 1;
+return b;
+}
+int foo() {
+  int a{};
+  auto b = extracted(a);
+  return b;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput2), HoistingOutput2);
+
+  std::string HoistingInput3 = R"cpp(
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+[[a = 123;
+return a + b;]]
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  std::string HoistingOutput3 = R"cpp(
+int extracted(int , int ) {
+a = 123;
+return a + b;
+}
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+return extracted(b, a);
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput3), HoistingOutput3);
+
+  std::string HoistingInput4 = R"cpp(
+struct A {
+  bool flag;
+  int val;
+};
+A bar();
+int foo(int b) {
+  int a = 0;
+  [[auto [flag, val] = bar();
+  int c = 4;
+  val = c + a;]]
+  return a + b + c + val;
+}
+  )cpp";
+  std::string HoistingOutput4 = R"cpp(
+struct A {
+  bool flag;
+  int val;
+};
+A bar();
+auto extracted(int ) {
+auto [flag, val] = bar();
+  int c = 4;
+  val = c + a;
+return std::pair{val, c};
+}
+int foo(int b) {
+  int a = 0;
+  auto [val, c] = extracted(a);
+  return a + b + c + val;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput4), HoistingOutput4);
+}
+
+TEST_F(ExtractFunctionTest, HoistingCXX11) {
+  ExtraArgs.emplace_back("-std=c++11");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_THAT(apply(HoistingInput), HasSubstr("unavailable"));
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a;
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 1;
+return b;
+}
+int foo() {
+  int a;
+  auto b = extracted(a);
+  return b;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput2), HoistingOutput2);
+}
+
+TEST_F(ExtractFunctionTest, HoistingCXX14) {
+  ExtraArgs.emplace_back("-std=c++14");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 477184.
5chmidti added a comment.

Fixup: rm added includes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,9 @@
 Miscellaneous
 ^
 
+- The extract function tweak gained support for hoisting, i.e. returning decls declared
+  inside the selection that are used outside of the selection.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -30,8 +30,9 @@
   EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
   // Partial statements aren't extracted.
   EXPECT_THAT(apply("int [[x = 0]];"), "unavailable");
-  // FIXME: Support hoisting.
-  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), "unavailable");
+
+  // Extract regions that require hoisting
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), HasSubstr("extracted"));
 
   // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
   // lead to break being included in the extraction zone.
@@ -192,6 +193,202 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, Hoisting) {
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  std::string HoistingOutput = R"cpp(
+auto extracted(int ) {
+int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;
+return std::tuple{x, y, z};
+}
+int foo() {
+  int a = 3;
+  auto [x, y, z] = extracted(a);
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput), HoistingOutput);
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a{};
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 1;
+return b;
+}
+int foo() {
+  int a{};
+  auto b = extracted(a);
+  return b;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput2), HoistingOutput2);
+
+  std::string HoistingInput3 = R"cpp(
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+[[a = 123;
+return a + b;]]
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  std::string HoistingOutput3 = R"cpp(
+int extracted(int , int ) {
+a = 123;
+return a + b;
+}
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+return extracted(b, a);
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput3), HoistingOutput3);
+
+  std::string HoistingInput4 = R"cpp(
+struct A {
+  bool flag;
+  int val;
+};
+A bar();
+int foo(int b) {
+  int a = 0;
+  [[auto [flag, val] = bar();
+  int c = 4;
+  val = c + a;]]
+  return a + b + c + val;
+}
+  )cpp";
+  std::string HoistingOutput4 = R"cpp(
+struct A {
+  bool flag;
+  int val;
+};
+A bar();
+auto extracted(int ) {
+auto [flag, val] = bar();
+  int c = 4;
+  val = c + a;
+return std::pair{val, c};
+}
+int foo(int b) {
+  int a = 0;
+  auto [val, c] = extracted(a);
+  return a + b + c + val;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput4), HoistingOutput4);
+}
+
+TEST_F(ExtractFunctionTest, HoistingCXX11) {
+  ExtraArgs.emplace_back("-std=c++11");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_THAT(apply(HoistingInput), HasSubstr("unavailable"));
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a;
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 1;
+return b;
+}
+int foo() {
+  int a;
+  auto b = extracted(a);
+  return b;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput2), HoistingOutput2);
+}
+
+TEST_F(ExtractFunctionTest, HoistingCXX14) {
+  ExtraArgs.emplace_back("-std=c++14");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  std::string HoistingOutput = R"cpp(
+auto 

[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti created this revision.
5chmidti added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
5chmidti requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Adds support to hoist variables declared inside the selected region
and used afterwards back out of the extraced function for later use.
Uses the explicit variable type if only one decl needs hoisting,
otherwise pair or tuple with auto return type deduction
(requires c++14) and a structured binding (requires c++17) or
explicitly unpacking the variables with get<>.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138499

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -78,6 +78,9 @@
 Miscellaneous
 ^
 
+- The extract function tweak gained support for hoisting, i.e. returning decls declared
+  inside the selection that are used outside of the selection.
+
 Improvements to clang-doc
 -
 
Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -30,8 +30,9 @@
   EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
   // Partial statements aren't extracted.
   EXPECT_THAT(apply("int [[x = 0]];"), "unavailable");
-  // FIXME: Support hoisting.
-  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), "unavailable");
+
+  // Extract regions that require hoisting
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), HasSubstr("extracted"));
 
   // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
   // lead to break being included in the extraction zone.
@@ -192,6 +193,202 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, Hoisting) {
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  std::string HoistingOutput = R"cpp(
+auto extracted(int ) {
+int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;
+return std::tuple{x, y, z};
+}
+int foo() {
+  int a = 3;
+  auto [x, y, z] = extracted(a);
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput), HoistingOutput);
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a{};
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 1;
+return b;
+}
+int foo() {
+  int a{};
+  auto b = extracted(a);
+  return b;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput2), HoistingOutput2);
+
+  std::string HoistingInput3 = R"cpp(
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+[[a = 123;
+return a + b;]]
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  std::string HoistingOutput3 = R"cpp(
+int extracted(int , int ) {
+a = 123;
+return a + b;
+}
+int foo(int b) {
+  int a{};
+  if (b == 42) {
+return extracted(b, a);
+  }
+  a = 456;
+  return a;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput3), HoistingOutput3);
+
+  std::string HoistingInput4 = R"cpp(
+struct A {
+  bool flag;
+  int val;
+};
+A bar();
+int foo(int b) {
+  int a = 0;
+  [[auto [flag, val] = bar();
+  int c = 4;
+  val = c + a;]]
+  return a + b + c + val;
+}
+  )cpp";
+  std::string HoistingOutput4 = R"cpp(
+struct A {
+  bool flag;
+  int val;
+};
+A bar();
+auto extracted(int ) {
+auto [flag, val] = bar();
+  int c = 4;
+  val = c + a;
+return std::pair{val, c};
+}
+int foo(int b) {
+  int a = 0;
+  auto [val, c] = extracted(a);
+  return a + b + c + val;
+}
+  )cpp";
+  EXPECT_EQ(apply(HoistingInput4), HoistingOutput4);
+}
+
+TEST_F(ExtractFunctionTest, HoistingCXX11) {
+  ExtraArgs.emplace_back("-std=c++11");
+  std::string HoistingInput = R"cpp(
+int foo() {
+  int a = 3;
+  [[int x = 39 + a;
+  ++x;
+  int y = x * 2;
+  int z = 4;]]
+  return x + y + z;
+}
+  )cpp";
+  EXPECT_THAT(apply(HoistingInput), HasSubstr("unavailable"));
+
+  std::string HoistingInput2 = R"cpp(
+int foo() {
+  int a;
+  [[int b = a + 1;]]
+  return b;
+}
+  )cpp";
+  std::string HoistingOutput2 = R"cpp(
+int extracted(int ) {
+int b = a + 

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 454292.
5chmidti added a comment.

lower c++ version requirement by making the fixes conditional on c++20 instead 
of the whole check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator-cpp11.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
@@ -0,0 +1,505 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+
+template 
+struct I {
+  int X;
+  bool operator==(const I&) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const I&, const I&) = default;
+};
+
+template 
+struct J {
+  int X;
+  bool operator==(const J&) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const J&, const J&) = default;
+};
+
+template 
+struct K {
+  int X;
+  bool operator==(const K&) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const K&, const K&) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator 

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 454287.
5chmidti added a comment.

- address all comments
- minor renames of variables/functions
- add missing & when accessing OptTokens value
- rebase onto current HEAD
- fix llvm::Optional member deprecation warnings
- add support to match when the parameter type explicitly instantiaties itself 
(i.e. const A& instead of const A& for template  struct A {...};)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
@@ -0,0 +1,505 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+
+template 
+struct I {
+  int X;
+  bool operator==(const I&) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const I&, const I&) = default;
+};
+
+template 
+struct J {
+  int X;
+  bool operator==(const J&) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const J&, const J&) = default;
+};
+
+template 
+struct K {
+  int X;
+  bool operator==(const K&) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const K&, const K&) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) 

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

I've got everything done locally, but I'm waiting on the decision on the bitset 
to update the diff, including some minor additional updates I will list then.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:56
+bool isComparisonOperator(OverloadedOperatorKind Kind) {
+  return llvm::is_contained(
+  std::initializer_list{

njames93 wrote:
> Could these not be represented as a bitset?
Yes. I changed this to use a bitset. While changing it I thought about the 
performance and came to these two solutions:
A) is straight forward but suffers from bad-ish code gen when doing -O0 (58 
instructions), on -O1 and up it's just 4 instructions.
```
bool isComparisonOperator(OverloadedOperatorKind Kind) {
  std::bitset BitSetCompOps{};
  BitSetCompOps.set(OO_Less);
  BitSetCompOps.set(OO_Greater);
  BitSetCompOps.set(OO_EqualEqual);
  BitSetCompOps.set(OO_ExclaimEqual);
  BitSetCompOps.set(OO_LessEqual);
  BitSetCompOps.set(OO_GreaterEqual);
  BitSetCompOps.set(OO_Spaceship);
  BitSetCompOps.set(OO_AmpAmp);
  BitSetCompOps.set(OO_PipePipe);
  return BitSetCompOps[Kind];
}
```

B) the same, but this emits much less code during -O0 (14 instructions) and for 
-O1 and up it's the same as A)
```
template 
constexpr size_t isComparisonOperatorGenerateBitset() {
  return ((static_cast(1U) << ComparisonKinds) | ...);
}

bool isComparisonOperator(OverloadedOperatorKind Kind) {
  constexpr static auto BitsetCompOps = isComparisonOperatorGenerateBitset<
  OO_Less, OO_Greater, OO_EqualEqual, OO_ExclaimEqual, OO_LessEqual,
  OO_GreaterEqual, OO_Spaceship, OO_AmpAmp, OO_PipePipe>();
  return BitsetCompOps & (static_cast(1U) << Kind);
}
```
See also https://godbolt.org/z/E9aeW67xb for both examples.
I think B) would be better, but I thought I'd ask before going with a parameter 
pack instead of the std::bitset.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:84
+
+static Expected getNode(const BoundNodes , StringRef ID) {
+  auto  = Nodes.getMap();

njames93 wrote:
> Should there ever be a case where this returns an error. If not can this 
> assert instead and just return a DynTypedNode?
I got this bit from the Transformer lib implementation, but you're right, there 
are no paths in this check that call getNode with an unbound ID.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:135-136
+};
+return invalidArgumentError(
+"constSpec invalid argument: not a CXXMethodDecl");
+  };

njames93 wrote:
> Is this error reachable? Again if not use unreachable.
> If it is reachable, is it possible to show a test case where this error is 
> expected and detected in the test?
Yes it's unreachable, constSpec get's only called on bound node "op" which is a 
CXXMethodDecl. Changed it to llvm_unreachable like the other unreachable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Gentle Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

Question: Should this check be extended to also include C.86 
:
 "Flag an operator==() for which the argument types differ; same for other 
comparison operators: !=, <, <=, >, and >=."? With a warning along the lines 
of: "comparison operators should be symmetric with respect to the parameters 
types" or "parameters of comparison operators should be of the same type" (or 
some other warning). Or should that be a new check? I think it fits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

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

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 441197.
5chmidti added a comment.

Address inlining isLanguageVersionSupported


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend constexpr bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  constexpr bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend constexpr bool operator==(const F &, const F &) = default;
+};
+

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 441196.
5chmidti added a comment.

Fixup for the same reason


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend constexpr bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  constexpr bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend constexpr bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 441194.
5chmidti added a comment.

Fixup addressing comments, missed changing the links for docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend constexpr bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  constexpr bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend constexpr bool operator==(const F &, const F &) = 

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 441193.
5chmidti added a comment.

Addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend constexpr bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  constexpr bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend constexpr bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 441186.
5chmidti added a comment.

rm wrong op from isComparisonOperator


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend constexpr bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  constexpr bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend constexpr bool operator==(const F &, const F &) = default;
+};
+
+template 

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 441185.
5chmidti added a comment.

rm unused function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend constexpr bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  constexpr bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend constexpr bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;

[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti created this revision.
5chmidti added reviewers: aaron.ballman, njames93, alexfh, LegalizeAdulthood, 
Eugene.Zelenko.
Herald added subscribers: carlosgalvezp, shchenz, kbarton, xazax.hun, mgorny, 
nemanjai.
Herald added a project: All.
5chmidti requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.
Herald added a project: clang-tools-extra.

Adds a check for C++ Core Guidelines "C.161: Use non-member functions for 
symmetric operators"
Produces a warning on member operators that are symmetric binary operators
and generates fixes for symmetric defaulted comparison operators to be changed
to friend operator functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128861

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-symmetric-binary-operator.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-symmetric-binary-operator.cpp
@@ -0,0 +1,481 @@
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-symmetric-binary-operator %t
+
+namespace std {
+class string {
+public:
+  friend bool operator!=(const string &, const string &) { return true; }
+  friend bool operator<(const string &, const string &) { return true; }
+};
+template 
+class vector {
+public:
+  int size() const;
+  const T [](int) const;
+  friend bool operator==(const vector &, const vector &) = default;
+};
+} // namespace std
+
+using size_t = unsigned long long;
+
+namespace test_base {
+struct A {
+  int X;
+  bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A&, const A&) = default;
+};
+
+struct A2 {
+  int X;
+  bool operator==(const A2 ) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const A2&, const A2&) = default;
+};
+
+struct B {
+  int X;
+  friend bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend bool operator==(const D &, const D &) = default;
+};
+
+struct E {
+  int X;
+  bool operator==(const E &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const E&, const E&) = default;
+};
+
+struct F {
+  int X;
+  friend bool operator==(const F &, const F &) = default;
+};
+
+template 
+struct G {
+  int X;
+  bool operator==(const G &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend bool operator==(const G&, const G&) = default;
+};
+
+template 
+struct H {
+  int X;
+  friend bool operator==(const H &, const H &) = default;
+};
+} // namespace test_base
+
+namespace test_constexpr {
+struct A {
+  int X;
+  constexpr bool operator==(const A &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const A&, const A&) = default;
+};
+
+struct B {
+  int X;
+  friend constexpr bool operator==(const B &, const B &) = default;
+};
+
+template 
+struct C {
+  int X;
+  constexpr bool operator==(const C &) const = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: symmetric binary operator should be a friend or free operator function [cppcoreguidelines-symmetric-binary-operator]
+  // CHECK-FIXES: friend constexpr bool operator==(const C&, const C&) = default;
+};
+
+template 
+struct D {
+  int X;
+  friend