[PATCH] D86581: [clang-format] Handle shifts within conditions

[Miguel Saldivar via Phabricator via cfe-commits]

Saldivarcher added a comment.

@MyDeveloperDay looks like it's in master, thanks for committing for me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@Saldivarcher this should be landed now, please validate


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc81dd3d159ab: [clang-format] Handle shifts within conditions 
(authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7565,6 +7565,21 @@
   verifyFormat("static_assert(is_convertible::value, \"AAA\");");
   verifyFormat("Constructor(A... a) : a_(X{std::forward(a)}...) {}");
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
+  verifyFormat("some_templated_type");
+}
+
+TEST_F(FormatTest, UnderstandsShiftOperators) {
+  verifyFormat("if (i < x >> 1)");
+  verifyFormat("while (i < x >> 1)");
+  verifyFormat("for (unsigned i = 0; i < i; ++i, v = v >> 1)");
+  verifyFormat("for (unsigned i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat(
+  "for (std::vector::iterator i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat("Foo.call>()");
+  verifyFormat("if (Foo.call>() == 0)");
+  verifyFormat("for (std::vector>::iterator i = 0; i < x >> 1; "
+   "++i, v = v >> 1)");
+  verifyFormat("if (w>, 1>::t)");
 }
 
 TEST_F(FormatTest, BitshiftOperatorWidth) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -56,6 +56,13 @@
  Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr, tok::kw_catch);
+}
+
 /// A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -108,6 +115,12 @@
 
 while (CurrentToken) {
   if (CurrentToken->is(tok::greater)) {
+// Try to do a better job at looking for ">>" within the condition of
+// a statement.
+if (CurrentToken->Next && CurrentToken->Next->is(tok::greater) &&
+Left->ParentBracket != tok::less &&
+isKeywordWithCondition(*Line.First))
+  return false;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 // In TT_Proto, we must distignuish between:
@@ -2768,13 +2781,6 @@
   Right.ParameterCount > 0);
 }
 
-/// Returns \c true if the token is followed by a boolean condition, \c false
-/// otherwise.
-static bool isKeywordWithCondition(const FormatToken &Tok) {
-  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
- tok::kw_constexpr, tok::kw_catch);
-}
-
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7565,6 +7565,21 @@
   verifyFormat("static_assert(is_convertible::value, \"AAA\");");
   verifyFormat("Constructor(A... a) : a_(X{std::forward(a)}...) {}");
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
+  verifyFormat("some_templated_type");
+}
+
+TEST_F(FormatTest, UnderstandsShiftOperators) {
+  verifyFormat("if (i < x >> 1)");
+  verifyFormat("while (i < x >> 1)");
+  verifyFormat("for (unsigned i = 0; i < i; ++i, v = v >> 1)");
+  verifyFormat("for (unsigned i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat(
+  "for (std::vector::iterator i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat("Foo.call>()");
+  verifyFormat("if (Foo.call>() == 0)");
+  verifyFormat("for (std::vector>::iterator i = 0; i < x >> 1; "
+   "++i, v = v >> 1)");
+  verifyFormat("if (w>, 1>::t)");
 }
 
 TEST_F(FormatTest, BitshiftOperatorWidth) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -56,6 +56,13 @@
  Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr, tok::k

[PATCH] D86581: [clang-format] Handle shifts within conditions

[Miguel Saldivar via Phabricator via cfe-commits]

Saldivarcher added a comment.

@MyDeveloperDay thanks for the kind words, it's much appreciated! I'm just glad 
I was able to help. Nope, I don't have commit access, can you do me the favor 
of commiting for me?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@Saldivarcher do you have commit access?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[MyDeveloperDay via Phabricator via cfe-commits]

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

I think this LGTM and the solution is much more elegant than mine! You were not 
stepping on my toes, I'm always happy for someone else to take things over and 
this is a much smarter solution than mine.

This bug has been reported multiple times I think this is a good first step 
towards a solution, and I don't think "perfection should be the enemy of good", 
I think for most cases this will suffice.

anything else more obtuse I wonder why people write code in that form when its 
hard for the non author to read, but we can address those cases when we hit 
them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[Miguel Saldivar via Phabricator via cfe-commits]

Saldivarcher added a comment.

MyDeveloperDay, I added your tests. Sorry to step on your toes, I didn't know 
there was a patch open for this already :(. Yeah, JakeMerdichAMD, it's kinda 
tough to handle those types of cases without some sort of parsing. I'm glad 
you're working on that, and my code doesn't get in the way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[Miguel Saldivar via Phabricator via cfe-commits]

Saldivarcher updated this revision to Diff 288013.
Saldivarcher added a comment.

Added some more tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7549,6 +7549,21 @@
   verifyFormat("static_assert(is_convertible::value, \"AAA\");");
   verifyFormat("Constructor(A... a) : a_(X{std::forward(a)}...) {}");
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
+  verifyFormat("some_templated_type");
+}
+
+TEST_F(FormatTest, UnderstandsShiftOperators) {
+  verifyFormat("if (i < x >> 1)");
+  verifyFormat("while (i < x >> 1)");
+  verifyFormat("for (unsigned i = 0; i < i; ++i, v = v >> 1)");
+  verifyFormat("for (unsigned i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat(
+  "for (std::vector::iterator i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat("Foo.call>()");
+  verifyFormat("if (Foo.call>() == 0)");
+  verifyFormat("for (std::vector>::iterator i = 0; i < x >> 1; "
+   "++i, v = v >> 1)");
+  verifyFormat("if (w>, 1>::t)");
 }
 
 TEST_F(FormatTest, BitshiftOperatorWidth) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -56,6 +56,13 @@
  Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr, tok::kw_catch);
+}
+
 /// A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -108,6 +115,12 @@
 
 while (CurrentToken) {
   if (CurrentToken->is(tok::greater)) {
+// Try to do a better job at looking for ">>" within the condition of
+// a statement.
+if (CurrentToken->Next && CurrentToken->Next->is(tok::greater) &&
+Left->ParentBracket != tok::less &&
+isKeywordWithCondition(*Line.First))
+  return false;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 // In TT_Proto, we must distignuish between:
@@ -2733,13 +2746,6 @@
   Right.ParameterCount > 0);
 }
 
-/// Returns \c true if the token is followed by a boolean condition, \c false
-/// otherwise.
-static bool isKeywordWithCondition(const FormatToken &Tok) {
-  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
- tok::kw_constexpr, tok::kw_catch);
-}
-
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7549,6 +7549,21 @@
   verifyFormat("static_assert(is_convertible::value, \"AAA\");");
   verifyFormat("Constructor(A... a) : a_(X{std::forward(a)}...) {}");
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
+  verifyFormat("some_templated_type");
+}
+
+TEST_F(FormatTest, UnderstandsShiftOperators) {
+  verifyFormat("if (i < x >> 1)");
+  verifyFormat("while (i < x >> 1)");
+  verifyFormat("for (unsigned i = 0; i < i; ++i, v = v >> 1)");
+  verifyFormat("for (unsigned i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat(
+  "for (std::vector::iterator i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat("Foo.call>()");
+  verifyFormat("if (Foo.call>() == 0)");
+  verifyFormat("for (std::vector>::iterator i = 0; i < x >> 1; "
+   "++i, v = v >> 1)");
+  verifyFormat("if (w>, 1>::t)");
 }
 
 TEST_F(FormatTest, BitshiftOperatorWidth) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -56,6 +56,13 @@
  Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr, tok::kw_catch);
+}
+
 /// A parser that gathers additional information about tokens.

[PATCH] D86581: [clang-format] Handle shifts within conditions

[Jake Merdich via Phabricator via cfe-commits]

JakeMerdichAMD added a comment.

Agreed that this is a very nice solution for this case. LGTM too assuming it 
passes @MyDeveloperDay's tests.

Minor heads up: there are still some cases I can dream up where we'd currently 
spit out invalid code from splitting a right shift even after this fix. Long 
story short, C++14 made some cases completely ambiguous and impossible to 
detect without semantic analysis (https://godbolt.org/z/rxEqTG). I'll be having 
a fix out for that soon, but it's rather complementary to this one-- yours 
improves template detection, mine acts more conservative in case we guessed 
wrong about it being a template.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I think I like your solution better than mine D79293: [clang-format] [PR45218]  
Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at 
a TemplateOpener/Closer , could you look at my 
unit tests and consider adding them here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86581

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

[PATCH] D86581: [clang-format] Handle shifts within conditions

[Miguel Saldivar via Phabricator via cfe-commits]

Saldivarcher created this revision.
Saldivarcher added a reviewer: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Saldivarcher requested review of this revision.

In some situation shifts can be treated as a template, and is thus formatted as 
one. So, by doing a couple extra checks to assure that the condition doesn't 
contain a template, and is in fact a bit shift should solve this problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86581

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7540,6 +7540,9 @@
"  while (a < b && c > d) {\n"
"  }\n"
"}");
+  verifyFormat("int j = 10;\n"
+   "for (int i = 0; i < j >> 1; i++)\n"
+   "  ;");
   verifyFormat("template \n"
"typename enable_if<0 < sizeof...(Types)>::type Foo() {}");
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -56,6 +56,13 @@
  Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr, tok::kw_catch);
+}
+
 /// A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -108,6 +115,12 @@
 
 while (CurrentToken) {
   if (CurrentToken->is(tok::greater)) {
+// Try to do a better job at looking for ">>" within the condition of
+// a statement.
+if (CurrentToken->Next && CurrentToken->Next->is(tok::greater) &&
+Left->ParentBracket != tok::less &&
+isKeywordWithCondition(*Line.First))
+  return false;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 // In TT_Proto, we must distignuish between:
@@ -2733,13 +2746,6 @@
   Right.ParameterCount > 0);
 }
 
-/// Returns \c true if the token is followed by a boolean condition, \c false
-/// otherwise.
-static bool isKeywordWithCondition(const FormatToken &Tok) {
-  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
- tok::kw_constexpr, tok::kw_catch);
-}
-
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7540,6 +7540,9 @@
"  while (a < b && c > d) {\n"
"  }\n"
"}");
+  verifyFormat("int j = 10;\n"
+   "for (int i = 0; i < j >> 1; i++)\n"
+   "  ;");
   verifyFormat("template \n"
"typename enable_if<0 < sizeof...(Types)>::type Foo() {}");
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -56,6 +56,13 @@
  Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr, tok::kw_catch);
+}
+
 /// A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -108,6 +115,12 @@
 
 while (CurrentToken) {
   if (CurrentToken->is(tok::greater)) {
+// Try to do a better job at looking for ">>" within the condition of
+// a statement.
+if (CurrentToken->Next && CurrentToken->Next->is(tok::greater) &&
+Left->ParentBracket != tok::less &&
+isKeywordWithCondition(*Line.First))
+  return false;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 // In TT_Proto, we must distignuish between:
@@ -2733,13 +2746,6 @@
   Right.ParameterCount > 0);
 }
 
-/// Returns \c true if the token is followed by a boolean condition, \c false
-/// otherwise.
-static bool isKeywordWithCondition(const FormatToken &Tok) {
-  return Tok.isOneOf