[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-21 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added inline comments.



Comment at: clang/lib/Format/WhitespaceManager.cpp:716-717
 break;
   if (Next->isOneOf(TT_StartOfName, TT_FunctionDeclarationName,
 tok::kw_operator))
 return false;

Maybe I should combine the conditions into this `isOneOf(...)` together since 
they all return false.

Well, when I was writing this, I remembered why I had put the check before line 
714. It was because line 714 would be executed and breaks in that scenario.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-22 Thread Darwin Xu via Phabricator via cfe-commits 
darwin updated this revision to Diff 325455.
darwin added a comment.

Add more test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13945,6 +13945,20 @@
   verifyFormat("int  oneTwoThree{0}; // comment\n"
"unsigned oneTwo; // comment",
Alignment);
+  verifyFormat("unsigned int *  a;\n"
+   "int *   b;\n"
+   "unsigned int Const *c;\n"
+   "unsigned int const *d;\n"
+   "unsigned int Const &e;\n"
+   "unsigned int const &f;",
+   Alignment);
+  verifyFormat("Const unsigned int *c;\n"
+   "const unsigned int *d;\n"
+   "Const unsigned int &e;\n"
+   "const unsigned int &f;\n"
+   "const unsigned  g;\n"
+   "Const unsigned  h;",
+Alignment);
   EXPECT_EQ("float const a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
@@ -14249,6 +14263,38 @@
   EXPECT_EQ("DECOR1 /**/ int8_t /**/ DECOR2 /**/\n"
 "foo(int a);",
 format("DECOR1 /**/ int8_t /**/ DECOR2 /**/ foo (int a);", Style));
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("unsigned int*   a;\n"
+   "int*b;\n"
+   "unsigned int Const* c;\n"
+   "unsigned int const* d;\n"
+   "unsigned int Const& e;\n"
+   "unsigned int const& f;",
+   Alignment);
+  verifyFormat("Const unsigned int* c;\n"
+   "const unsigned int* d;\n"
+   "Const unsigned int& e;\n"
+   "const unsigned int& f;\n"
+   "const unsigned  g;\n"
+   "Const unsigned  h;",
+Alignment);
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("unsigned int *   a;\n"
+   "int *b;\n"
+   "unsigned int Const * c;\n"
+   "unsigned int const * d;\n"
+   "unsigned int Const & e;\n"
+   "unsigned int const & f;",
+   Alignment);
+  verifyFormat("Const unsigned int * c;\n"
+   "const unsigned int * d;\n"
+   "Const unsigned int & e;\n"
+   "const unsigned int & f;\n"
+   "const unsigned   g;\n"
+   "Const unsigned   h;",
+Alignment);
 }
 
 TEST_F(FormatTest, LinuxBraceBreaking) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -709,6 +709,8 @@
 for (FormatToken *Next = C.Tok->Next; Next; Next = Next->Next) {
   if (Next->is(tok::comment))
 continue;
+  if (Next->is(TT_PointerOrReference))
+return false;
   if (!Next->Tok.getIdentifierInfo())
 break;
   if (Next->isOneOf(TT_StartOfName, TT_FunctionDeclarationName,


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13945,6 +13945,20 @@
   verifyFormat("int  oneTwoThree{0}; // comment\n"
"unsigned oneTwo; // comment",
Alignment);
+  verifyFormat("unsigned int *  a;\n"
+   "int *   b;\n"
+   "unsigned int Const *c;\n"
+   "unsigned int const *d;\n"
+   "unsigned int Const &e;\n"
+   "unsigned int const &f;",
+   Alignment);
+  verifyFormat("Const unsigned int *c;\n"
+   "const unsigned int *d;\n"
+   "Const unsigned int &e;\n"
+   "const unsigned int &f;\n"
+   "const unsigned  g;\n"
+   "Const unsigned  h;",
+Alignment);
   EXPECT_EQ("float const a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
@@ -14249,6 +14263,38 @@
   EXPECT_EQ("DECOR1 /**/ int8_t /**/ DECOR2 /**/\n"
 "foo(int a);",
 format("DECOR1 /**/ int8_t /**/ DECOR2 /**/ foo (int a);", Style));
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("unsigned int*   a;\n"
+   "int*b;\n"
+   "unsigned int Const* c;\n"
+   "unsigned int const* d;\n"
+   "unsigned int Const& e;\n"
+   "unsigned int const& f;",
+   Alignment);
+  verifyFormat("Const

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-22 Thread Björn Schäpers via Phabricator via cfe-commits 
HazardyKnusperkeks added a comment.

You should mark comments as done, if they are.

Does your modification maybe add something to the alignment which is not a 
declaration?

  int a;
  double b;
  a * b;

How is that formatted? Yeah unlikely that something like that is in code, but 
it could be if `operator*` has side effects and one does not need the result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-22 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D97137#2579669 , 
@HazardyKnusperkeks wrote:

> You should mark comments as done, if they are.
>
> Does your modification maybe add something to the alignment which is not a 
> declaration?
>
>   int a;
>   double b;
>   a * b;
>
> How is that formatted? Yeah unlikely that something like that is in code, but 
> it could be if `operator*` has side effects and one does not need the result.

Good question.

I've tested the original code and the modified code, both will generate the 
same result:

  inta;
  double b;
  a* b;

I understand the expected format should be:

  inta;
  double b;
  a* b;

Maybe we can register another bug to track it.

For the side effects, I couldn't answer this yet since I am no expert. Can 
someone take a deep look of it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-23 Thread Darwin Xu via Phabricator via cfe-commits 
darwin updated this revision to Diff 325766.
darwin marked 3 inline comments as done.
darwin added a comment.

Fix the code's alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13945,6 +13945,20 @@
   verifyFormat("int  oneTwoThree{0}; // comment\n"
"unsigned oneTwo; // comment",
Alignment);
+  verifyFormat("unsigned int *  a;\n"
+   "int *   b;\n"
+   "unsigned int Const *c;\n"
+   "unsigned int const *d;\n"
+   "unsigned int Const &e;\n"
+   "unsigned int const &f;",
+   Alignment);
+  verifyFormat("Const unsigned int *c;\n"
+   "const unsigned int *d;\n"
+   "Const unsigned int &e;\n"
+   "const unsigned int &f;\n"
+   "const unsigned  g;\n"
+   "Const unsigned  h;",
+   Alignment);
   EXPECT_EQ("float const a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
@@ -14249,6 +14263,38 @@
   EXPECT_EQ("DECOR1 /**/ int8_t /**/ DECOR2 /**/\n"
 "foo(int a);",
 format("DECOR1 /**/ int8_t /**/ DECOR2 /**/ foo (int a);", Style));
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("unsigned int*   a;\n"
+   "int*b;\n"
+   "unsigned int Const* c;\n"
+   "unsigned int const* d;\n"
+   "unsigned int Const& e;\n"
+   "unsigned int const& f;",
+   Alignment);
+  verifyFormat("Const unsigned int* c;\n"
+   "const unsigned int* d;\n"
+   "Const unsigned int& e;\n"
+   "const unsigned int& f;\n"
+   "const unsigned  g;\n"
+   "Const unsigned  h;",
+   Alignment);
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("unsigned int *   a;\n"
+   "int *b;\n"
+   "unsigned int Const * c;\n"
+   "unsigned int const * d;\n"
+   "unsigned int Const & e;\n"
+   "unsigned int const & f;",
+   Alignment);
+  verifyFormat("Const unsigned int * c;\n"
+   "const unsigned int * d;\n"
+   "Const unsigned int & e;\n"
+   "const unsigned int & f;\n"
+   "const unsigned   g;\n"
+   "Const unsigned   h;",
+   Alignment);
 }
 
 TEST_F(FormatTest, LinuxBraceBreaking) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -709,6 +709,8 @@
 for (FormatToken *Next = C.Tok->Next; Next; Next = Next->Next) {
   if (Next->is(tok::comment))
 continue;
+  if (Next->is(TT_PointerOrReference))
+return false;
   if (!Next->Tok.getIdentifierInfo())
 break;
   if (Next->isOneOf(TT_StartOfName, TT_FunctionDeclarationName,


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13945,6 +13945,20 @@
   verifyFormat("int  oneTwoThree{0}; // comment\n"
"unsigned oneTwo; // comment",
Alignment);
+  verifyFormat("unsigned int *  a;\n"
+   "int *   b;\n"
+   "unsigned int Const *c;\n"
+   "unsigned int const *d;\n"
+   "unsigned int Const &e;\n"
+   "unsigned int const &f;",
+   Alignment);
+  verifyFormat("Const unsigned int *c;\n"
+   "const unsigned int *d;\n"
+   "Const unsigned int &e;\n"
+   "const unsigned int &f;\n"
+   "const unsigned  g;\n"
+   "Const unsigned  h;",
+   Alignment);
   EXPECT_EQ("float const a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
@@ -14249,6 +14263,38 @@
   EXPECT_EQ("DECOR1 /**/ int8_t /**/ DECOR2 /**/\n"
 "foo(int a);",
 format("DECOR1 /**/ int8_t /**/ DECOR2 /**/ foo (int a);", Style));
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("unsigned int*   a;\n"
+   "int*b;\n"
+   "unsigned int Const* c;\n"
+   "unsigned int const* d;\n"
+   "unsigned int Const& e;\n"
+   "unsigned int const& f;",
+

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-23 Thread Björn Schäpers via Phabricator via cfe-commits 
HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

In D97137#2580664 , @darwin wrote:

> In D97137#2579669 , 
> @HazardyKnusperkeks wrote:
>
>> You should mark comments as done, if they are.
>>
>> Does your modification maybe add something to the alignment which is not a 
>> declaration?
>>
>>   int a;
>>   double b;
>>   a * b;
>>
>> How is that formatted? Yeah unlikely that something like that is in code, 
>> but it could be if `operator*` has side effects and one does not need the 
>> result.
>
> Good question.
>
> I've tested the original code and the modified code, both will generate the 
> same result:
>
>   inta;
>   double b;
>   a* b;
>
> I understand the expected format should be:
>
>   inta;
>   double b;
>   a* b;
>
> Maybe we can register another bug to track it.

If it was formatted like that before, everything is fine by me. clang-format 
does not know the types (or if it are types) of `a` and `b`.

> For the side effects, I couldn't answer this yet since I am no expert. Can 
> someone take a deep look of it?

If all tests pass this is fine for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-24 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

Hi guys, thanks for accepting the change. But I don't have have commit access 
of LLVM. Can someone commit it for me?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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

[PATCH] D97137: Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly

2021-02-25 Thread Björn Schäpers via Phabricator via cfe-commits 
HazardyKnusperkeks added a comment.

In D97137#2584417 , @darwin wrote:

> Hi guys, thanks for accepting the change. But I don't have commit access of 
> LLVM. Can someone commit it for me?

Can and will do. Need the name and email for the commit.

But I will wait a bit, if someone raises a concern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97137

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