[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Malcolm Parsons via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL285934: Fixed column shift when formatting line containing 
bit shift operators (authored by malcolm.parsons).

Changed prior to commit:
  https://reviews.llvm.org/D25439?vs=75635=76869#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25439

Files:
  cfe/trunk/lib/Format/FormatTokenLexer.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/lib/Format/FormatTokenLexer.cpp
===
--- cfe/trunk/lib/Format/FormatTokenLexer.cpp
+++ cfe/trunk/lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -5501,6 +5501,18 @@
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
 }
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  EXPECT_EQ("int a = 1 << 2; /* foo\n"
+"   bar */",
+format("inta=1<<2;  /* foo\n"
+   "   bar */"));
+
+  EXPECT_EQ("int b = 256 >> 1; /* foo\n"
+" bar */",
+format("int  b  =256>>1 ;  /* foo\n"
+   "  bar */"));
+}
+
 TEST_F(FormatTest, UnderstandsBinaryOperators) {
   verifyFormat("COMPARE(a, ==, b);");
   verifyFormat("auto s = sizeof...(Ts) - 1;");


Index: cfe/trunk/lib/Format/FormatTokenLexer.cpp
===
--- cfe/trunk/lib/Format/FormatTokenLexer.cpp
+++ cfe/trunk/lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -5501,6 +5501,18 @@
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
 }
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  EXPECT_EQ("int a = 1 << 2; /* foo\n"
+"   bar */",
+format("inta=1<<2;  /* foo\n"
+   "   bar */"));
+
+  EXPECT_EQ("int b = 256 >> 1; /* foo\n"
+" bar */",
+format("int  b  =256>>1 ;  /* foo\n"
+   "  bar */"));
+}
+
 TEST_F(FormatTest, UnderstandsBinaryOperators) {
   verifyFormat("COMPARE(a, ==, b);");
   verifyFormat("auto s = sizeof...(Ts) - 1;");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Paweł Żukowski via cfe-commits]

idlecode added a comment.

No, not yet - I was about to ask someone to commit this for me :)


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Malcolm Parsons via cfe-commits]

malcolm.parsons added a comment.

Do you have commit access?


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Daniel Jasper via cfe-commits]

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

Looks good. Thank you!


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Paweł Żukowski via cfe-commits]

idlecode updated this revision to Diff 75635.

https://reviews.llvm.org/D25439

Files:
  lib/Format/FormatTokenLexer.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5501,6 +5501,18 @@
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
 }
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  EXPECT_EQ("int a = 1 << 2; /* foo\n"
+"   bar */",
+format("inta=1<<2;  /* foo\n"
+   "   bar */"));
+
+  EXPECT_EQ("int b = 256 >> 1; /* foo\n"
+" bar */",
+format("int  b  =256>>1 ;  /* foo\n"
+   "  bar */"));
+}
+
 TEST_F(FormatTest, UnderstandsBinaryOperators) {
   verifyFormat("COMPARE(a, ==, b);");
   verifyFormat("auto s = sizeof...(Ts) - 1;");
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5501,6 +5501,18 @@
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
 }
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  EXPECT_EQ("int a = 1 << 2; /* foo\n"
+"   bar */",
+format("inta=1<<2;  /* foo\n"
+   "   bar */"));
+
+  EXPECT_EQ("int b = 256 >> 1; /* foo\n"
+" bar */",
+format("int  b  =256>>1 ;  /* foo\n"
+   "  bar */"));
+}
+
 TEST_F(FormatTest, UnderstandsBinaryOperators) {
   verifyFormat("COMPARE(a, ==, b);");
   verifyFormat("auto s = sizeof...(Ts) - 1;");
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+++Column;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Paweł Żukowski via cfe-commits]

idlecode added a comment.

Thanks for pointing it out, just a minute ago I found a proper document 
 mentioning it (I have no idea how I 
could miss it).
I hope to be more use in future :)




Comment at: unittests/Format/FormatTest.cpp:11365
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  std::string left = "int a = 1 << 2; /* foo\n"
+ "   bar */";

djasper wrote:
> It's always useful to have some other formatting being done in the same test. 
> We repeatedly ran into cases in the past where a test only passed because 
> some change effectively disabled formatting for a specific line. I suggest 
> writing these as:
> 
>   EXPECT_EQ("int a = 1 << 2; /* foo\n"
> "   bar */",
> format("inta=1<<2;  /* foo\n"
>"   bar */"));
Oh, that is worth mentioning, thanks :)



Comment at: unittests/Format/FormatTest.cpp:11365
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  std::string left = "int a = 1 << 2; /* foo\n"
+ "   bar */";

idlecode wrote:
> djasper wrote:
> > It's always useful to have some other formatting being done in the same 
> > test. We repeatedly ran into cases in the past where a test only passed 
> > because some change effectively disabled formatting for a specific line. I 
> > suggest writing these as:
> > 
> >   EXPECT_EQ("int a = 1 << 2; /* foo\n"
> > "   bar */",
> > format("inta=1<<2;  /* foo\n"
> >"   bar */"));
> Oh, that is worth mentioning, thanks :)
Oh, that is good to know; Done


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Daniel Jasper via cfe-commits]

djasper added a comment.

Generally, always upload diffs with the full file as context to phabricator. 
That way, it is easier to see how the diff fits into the rest of the file. 
Thanks for fixing this!!




Comment at: lib/Format/FormatTokenLexer.cpp:528
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);

++Column;



Comment at: lib/Format/FormatTokenLexer.cpp:533
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);

++Column;



Comment at: unittests/Format/FormatTest.cpp:11364
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  std::string left = "int a = 1 << 2; /* foo\n"

Could you add this right underneath the test "UnderstandsTemplateParameters"?



Comment at: unittests/Format/FormatTest.cpp:11365
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  std::string left = "int a = 1 << 2; /* foo\n"
+ "   bar */";

It's always useful to have some other formatting being done in the same test. 
We repeatedly ran into cases in the past where a test only passed because some 
change effectively disabled formatting for a specific line. I suggest writing 
these as:

  EXPECT_EQ("int a = 1 << 2; /* foo\n"
"   bar */",
format("inta=1<<2;  /* foo\n"
   "   bar */"));


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Paweł Żukowski via cfe-commits]

idlecode updated this revision to Diff 74412.

https://reviews.llvm.org/D25439

Files:
  lib/Format/FormatTokenLexer.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11361,6 +11361,16 @@
"llvm::outs()\n<<");
 }
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  std::string left = "int a = 1 << 2; /* foo\n"
+ "   bar */";
+  EXPECT_EQ(left, format(left));
+
+  std::string right = "int b = 256 >> 2; /* foo\n"
+  " bar */";
+  EXPECT_EQ(right, format(right));
+}
+
 TEST_F(FormatTest, HandleUnbalancedImplicitBracesAcrossPPBranches) {
   std::string code = "#if A\n"
  "#if B\n"
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11361,6 +11361,16 @@
"llvm::outs()\n<<");
 }
 
+TEST_F(FormatTest, BitshiftOperatorWidth) {
+  std::string left = "int a = 1 << 2; /* foo\n"
+ "   bar */";
+  EXPECT_EQ(left, format(left));
+
+  std::string right = "int b = 256 >> 2; /* foo\n"
+  " bar */";
+  EXPECT_EQ(right, format(right));
+}
+
 TEST_F(FormatTest, HandleUnbalancedImplicitBracesAcrossPPBranches) {
   std::string code = "#if A\n"
  "#if B\n"
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Paweł Żukowski via cfe-commits]

idlecode marked an inline comment as done.
idlecode added a comment.

Sure, I will upload diff as soon as I test it


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Daniel Jasper via cfe-commits]

djasper added inline comments.



Comment at: test/Format/bitshift-operator-width.cpp:1
+// RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=LLVM \
+// RUN:   | FileCheck -strict-whitespace %s

Could you move this test into unittests/Format/FormatTest.cpp?


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Martin Probst via cfe-commits]

mprobst added a comment.

Looks good to me, but Daniel is a better owner for this code.


https://reviews.llvm.org/D25439



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

[PATCH] D25439: Fixed column shift when formatting line containing bit shift operators

[Paweł Żukowski via cfe-commits]

idlecode created this revision.
idlecode added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

During clang-format source lexing >> and << operators are split and
treated as two less/greater operators but column position of following
tokens was not adjusted accordingly.


https://reviews.llvm.org/D25439

Files:
  lib/Format/FormatTokenLexer.cpp
  test/Format/bitshift-operator-width.cpp


Index: test/Format/bitshift-operator-width.cpp
===
--- /dev/null
+++ test/Format/bitshift-operator-width.cpp
@@ -0,0 +1,13 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=LLVM \
+// RUN:   | FileCheck -strict-whitespace %s
+
+// CHECK: {{^int a = 1 << 2; /\* foo$}}
+// CHECK: {{^   bar \*/$}}
+int a = 1 << 2; /* foo
+   bar */
+
+// CHECK: {{^int b = 256 >> 2; /\* foo$}}
+// CHECK: {{^ bar \*/$}}
+int b = 256 >> 2; /* foo
+ bar */
+
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 


Index: test/Format/bitshift-operator-width.cpp
===
--- /dev/null
+++ test/Format/bitshift-operator-width.cpp
@@ -0,0 +1,13 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s | clang-format -style=LLVM \
+// RUN:   | FileCheck -strict-whitespace %s
+
+// CHECK: {{^int a = 1 << 2; /\* foo$}}
+// CHECK: {{^   bar \*/$}}
+int a = 1 << 2; /* foo
+   bar */
+
+// CHECK: {{^int b = 256 >> 2; /\* foo$}}
+// CHECK: {{^ bar \*/$}}
+int b = 256 >> 2; /* foo
+ bar */
+
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -525,10 +525,12 @@
   } else if (FormatTok->Tok.is(tok::greatergreater)) {
 FormatTok->Tok.setKind(tok::greater);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   } else if (FormatTok->Tok.is(tok::lessless)) {
 FormatTok->Tok.setKind(tok::less);
 FormatTok->TokenText = FormatTok->TokenText.substr(0, 1);
+Column += 1;
 StateStack.push(LexerState::TOKEN_STASHED);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits