[PATCH] D97137: Bug fix of https://bugs.llvm.org/show_bug.cgi?id=49175

2021-02-20 Thread Darwin Xu via Phabricator via cfe-commits 
darwin created this revision.
darwin added a reviewer: clang-format.
darwin added a project: clang-format.
darwin requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The AlignConsecutiveDeclarations option doesn't handle pointer properly:

The expected code format:

  unsigned int*   a;
  int*b;
  unsigned int Const* c;

The actual code after formatting:

  unsigned int* a;
  int*  b;
  unsigned int Const* c;

>From the code of clang-format, it seems the WhitespaceManager miss treated 
>`Const` as one of the token and which leads to the faulty behavior. So I added 
>an extra check that if the next token is a pointer or a reference, then the 
>current token is not the aligned token (the `matcher` lambda returns false).

Unit test passed:

  darwin@Darwins-iMac build % cmake --build . -j24 -t check-clang-unit
  ...
  [100%] Running lit suite /Volumes/silo/Projects/llvm-project/clang/test/Unit
  
  Testing Time: 270.81s
Passed: 13848
  [100%] Built target check-clang-unit


Repository:
  rG LLVM Github Monorepo

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,10 @@
   verifyFormat("int  oneTwoThree{0}; // comment\n"
"unsigned oneTwo; // comment",
Alignment);
+  verifyFormat("unsigned int *  a;\n"
+   "int *   b;\n"
+   "unsigned int Const *c;",
+   Alignment);
   EXPECT_EQ("float const a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
@@ -14249,6 +14253,12 @@
   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;",
+   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,10 @@
   verifyFormat("int  oneTwoThree{0}; // comment\n"
"unsigned oneTwo; // comment",
Alignment);
+  verifyFormat("unsigned int *  a;\n"
+   "int *   b;\n"
+   "unsigned int Const *c;",
+   Alignment);
   EXPECT_EQ("float const a = 5;\n"
 "\n"
 "int oneTwoThree = 123;",
@@ -14249,6 +14253,12 @@
   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;",
+   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,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97137: Bug fix of https://bugs.llvm.org/show_bug.cgi?id=49175

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

In D97137#2577794 , 
@HazardyKnusperkeks wrote:

> Just out of curiosity, in which language is `Const` used?

For example it is C's macro.

I found this issue in our C code. It has many macros and one function was 
defined like this:

  static IO_FLT_POSTOP_CALLBACK_STATUS FilterDriverClosePostOpCallback(
  IN OUT PIO_FLT_POSTOP_DATA  pPostopData,
  IN IO_FLT_RELATED_OBJECTS   FltObjects,
  IN OPTIONAL PIO_FLT_CONTEXT pCompletionContext,
  IN IO_FLT_POST_OPERATION_FLAGS  Flags,
  OUT PIO_FLT_POSTOP_CANCEL_CALLBACK* pCancellationCallback,
  OUT OPTIONAL PVOID* ppCancellationContext)

And because of this issue, the last two lines couldn't be formatted properly:

  static IO_FLT_POSTOP_CALLBACK_STATUS FilterDriverClosePostOpCallback(
  IN OUT PIO_FLT_POSTOP_DATA pPostopData,
  IN IO_FLT_RELATED_OBJECTS  FltObjects,
  IN OPTIONAL PIO_FLT_CONTEXTpCompletionContext,
  IN IO_FLT_POST_OPERATION_FLAGS Flags,
  OUT PIO_FLT_POSTOP_CANCEL_CALLBACK* pCancellationCallback,
  OUT OPTIONAL PVOID* ppCancellationContext)

Then, I started to investigate this issue. Interestingly, if the token before 
the asterisk is a keyword like `const` or `int`, it works fine sometimes:

  static IO_FLT_POSTOP_CALLBACK_STATUS FilterDriverClosePostOpCallback(
  IN OUT PIO_FLT_POSTOP_DATA pPostopData,
  IN IO_FLT_RELATED_OBJECTS  FltObjects,
  IN OPTIONAL PIO_FLT_CONTEXTpCompletionContext,
  IN IO_FLT_POST_OPERATION_FLAGS Flags,
  OUT int*   pCancellationCallback, 
<== I've changed PIO_FLT_POSTOP_CANCEL_CALLBACK into int, it could be 
formatted properly.
  OUT OPTIONAL const* ppCancellationContext)
<== I've changed PVOID into const, it still couldn't be formatted properly.

I didn't know the reason behind of this. Anyway, after this modification, it 
can handle all these types of code.




Comment at: clang/unittests/Format/FormatTest.cpp:14257
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("unsigned int*   a;\n"

curdeius wrote:
> Could you please test as well mixed pointers and references?
Sure.



Comment at: clang/unittests/Format/FormatTest.cpp:14260
+   "int*b;\n"
+   "unsigned int Const* c;",
+   Alignment);

curdeius wrote:
> Could you please test `const` as well?
Sure.

Actually, clang-format worked well for `const` originally.



Comment at: clang/unittests/Format/FormatTest.cpp:14261
+   "unsigned int Const* c;",
+   Alignment);
 }

MyDeveloperDay wrote:
> can you test west const too?
> 
> ```
> Const unsigned int* c;
> const unsigned int* d;
> Const unsigned int& e;
> const unsigned int& f;
> const unsigned g;
> Const unsigned h;
> ```
> 
Sure.


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-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 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-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] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-10 Thread Darwin Xu via Phabricator via cfe-commits 
darwin created this revision.
darwin added a project: clang-format.
darwin requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This a bug fix of https://bugs.llvm.org/show_bug.cgi?id=50116

First revision only contains the change of the test code. I would like to have 
someone review the expected result. If it is OK. Then I can fix it.

Here is the output of the test case without fixing yet:

  darwin@Darwins-iMac build % 
/Volumes/silo/Projects/open-source/llvm-project/build/tools/clang/unittests/Format/./FormatTests
 --gtest_filter=FormatTest.RemovesEmptyLines
  Note: Google Test filter = FormatTest.RemovesEmptyLines
  [==] Running 1 test from 1 test suite.
  [--] Global test environment set-up.
  [--] 1 test from FormatTest
  [ RUN  ] FormatTest.RemovesEmptyLines
  
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:279:
 Failure
  Expected equality of these values:
"namespace N\n" "{\n" "\n" "int i;\n" "}"
  Which is: "namespace N\n{\n\nint i;\n}"
format("namespace N\n" "{\n" "\n" "\n" "inti;\n" "}", 
GoogleWrapBraceAfterNamespace)
  Which is: "namespace N\n{\nint i;\n}"
  With diff:
  @@ -1,5 @@
   namespace N
   {
  -
   int i;
   }
  
  
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:290:
 Failure
  Expected equality of these values:
"/* something */ namespace N\n" "{\n" "\n" "int i;\n" "}"
  Which is: "/* something */ namespace N\n{\n\nint i;\n}"
format("/* something */ namespace N {\n" "\n" "\n" "inti;\n" "}", 
GoogleWrapBraceAfterNamespace)
  Which is: "/* something */ namespace N\n{\nint i;\n}"
  With diff:
  @@ -1,5 @@
   /* something */ namespace N
   {
  -
   int i;
   }
  
  
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:302:
 Failure
  Expected equality of these values:
"inline namespace N\n" "{\n" "\n" "int i;\n" "}"
  Which is: "inline namespace N\n{\n\nint i;\n}"
format("inline namespace N\n" "{\n" "\n" "\n" "inti;\n" "}", 
GoogleWrapBraceAfterNamespace)
  Which is: "inline namespace N\n{\nint i;\n}"
  With diff:
  @@ -1,5 @@
   inline namespace N
   {
  -
   int i;
   }
  
  
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:313:
 Failure
  Expected equality of these values:
"/* something */ inline namespace N\n" "{\n" "\n" "int i;\n" "}"
  Which is: "/* something */ inline namespace N\n{\n\nint i;\n}"
format("/* something */ inline namespace N\n" "{\n" "\n" "inti;\n" "}", 
GoogleWrapBraceAfterNamespace)
  Which is: "/* something */ inline namespace N\n{\nint i;\n}"
  With diff:
  @@ -1,5 @@
   /* something */ inline namespace N
   {
  -
   int i;
   }
  
  
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:324:
 Failure
  Expected equality of these values:
"export namespace N\n" "{\n" "\n" "int i;\n" "}"
  Which is: "export namespace N\n{\n\nint i;\n}"
format("export namespace N\n" "{\n" "\n" "inti;\n" "}", 
GoogleWrapBraceAfterNamespace)
  Which is: "export namespace N\n{\nint i;\n}"
  With diff:
  @@ -1,5 @@
   export namespace N
   {
  -
   int i;
   }
  
  
/Volumes/silo/Projects/open-source/llvm-project/clang/unittests/Format/FormatTest.cpp:345:
 Failure
  Expected equality of these values:
"namespace a\n" "{\n" "namespace b\n" "{\n" "\n" "class AA {};\n" "\n" "}  
// namespace b\n" "}  // namespace a\n"
  Which is: "namespace a\n{\nnamespace b\n{\n\nclass AA {};\n\n}  // 
namespace b\n}  // namespace a\n"
format("namespace a\n" "{\n" "namespace b\n" "{\n" "\n" "\n" "class AA 
{};\n" "\n" "\n" "}\n" "}\n", GoogleWrapBraceAfterNamespace)
  Which is: "namespace a\n{\nnamespace b\n{\nclass AA {};\n\n}  // 
namespace b\n}  // namespace a\n"
  With diff:
  @@ -3,5 @@
   namespace b
   {
  -
   class AA {};
   
  
  [  FAILED  ] FormatTest.RemovesEmptyLines (41 ms)
  [--] 1 test from FormatTest (41 ms total)
  
  [--] Global test environment tear-down
  [==] 1 test from 1 test suite ran. (41 ms total)
  [  PASSED  ] 0 tests.
  [  FAILED  ] 1 test, listed below:
  [  FAILED  ] FormatTest.RemovesEmptyLines
  
   1 FAILED TEST
  darwin@Darwins-iMac build % 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104044

Files:
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -262,6 +262,88 @@
"}",
getGoogleStyle()));
 
+  auto GoogleWrapBraceAfterNamespace = getGoogleStyle();
+  GoogleWrapBraceAfterNamespace.BreakBeforeBraces = FormatStyle::BS_Custom;
+  GoogleWrapBraceAfterNamespace.BraceWrapping.AfterNamespace = true;
+  EXPECT_EQ("namespace N\n"

[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-10 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

Sorry, I may need some help here. It shows "Context not available.", how do I 
correct it?


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-10 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2811268 , 
@HazardyKnusperkeks wrote:

> In D104044#2810909 , @darwin wrote:
>
>> Sorry, I may need some help here. It shows "Context not available.", how do 
>> I correct it?
>
> There are multiple ways: 
> https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
> I use `git diff HEAD~1 -U99 > mypatch.patch`
>
> For your tests: You want to keep an empty line after the now wrapped `{`, did 
> I understand that correctly?
> Is that bound to the google style, i.e. does it not happen with LLVM style?

Oh, now I remember, I forget to use this command. Thank you.

About the issue, let me explain it. It isn't bound to the google style or LLVM 
style either, since both of them keep the first brace at the same line of the 
namespace.

Let's see this example:

  darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
  namespace A{ int i; }
  
  namespace B{
  
  
  int j;
  
  
  }
  
  darwin@darwin-ubuntu-04:~/temp$ 
/home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp 
-style="{BasedOnStyle: google}"
  namespace A {
  int i;
  }
  
  namespace B {
  
  int j;
  
  }

You can see, if there isn't an empty line, clang-format doesn't add one. And if 
there are some extra lines, clang-format will keep just one line. This is 
expected.

But if I use set `BraceWrapping.AfterNamespace` to true, the thing is 
different, look this:

  darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
  namespace A{ int i; }
  
  namespace B{
  
  
  int j;
  
  
  }
  
  darwin@darwin-ubuntu-04:~/temp$ 
/home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp 
-style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: 
{AfterNamespace: true}}"
  namespace A
  {
  int i;
  }
  
  namespace B
  {
  int j;
  
  }

There isn't an empty line between the `{` and the `int j;`, but in the previous 
example, there is an empty line.

But later I found out that if I set `KeepEmptyLinesAtTheStartOfBlocks` to true, 
I will get the expect result:

  darwin@darwin-ubuntu-04:~/temp$ cat b.cpp
  namespace A{ int i; }
  
  namespace B{
  
  
  int j;
  
  
  }
  
  darwin@darwin-ubuntu-04:~/temp$ 
/home/darwin/projects/llvm-project/build/bin/clang-format  b.cpp 
-style="{BasedOnStyle: google, BreakBeforeBraces: Custom, BraceWrapping: 
{AfterNamespace: true}, KeepEmptyLinesAtTheStartOfBlocks: true}"
  namespace A
  {
  int i;
  }
  
  namespace B
  {
  
  int j;
  
  }

So, this might not be a very critical issue. But still, I think clang-format 
isn't working correctly and should be fixed.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-11 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2812612 , @MyDeveloperDay 
wrote:

> Just so I'm clear are you proposing that a newlines should always be added or 
> that single blank lines should be ignored? I can't tell if the bug is that 
> the line isn't being removed or sometimes not being added, either way there 
> will be someone who wants it the other way around correct?

If there is no linefeed at all, or there is only one line feed. The 
clang-format works fine.

The issue is, if there are empty lines between the line of namespace and the 
first line of statement, those empty lines will be removed.

For example, I want to to have code like this:

  01 namespace MyLibrary
  02 {
  03
  04 class Tool
  05 {
  06 };
  07 
  08 }

There is an empty line 03.

If I format this with clang-format, I will got this:

  01 namespace MyLibrary
  02 {
  03 class Tool
  04 {
  05 };
  06 
  07 }

The empty line between line 02 and line 03 is removed. This asymmetry is very 
annoying.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-11 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2813491 , @MyDeveloperDay 
wrote:

> Devils advocate how is this any different from
>
>   class Foo {
>   
>   class Bar {} ;
>   }
>   
>   };
>
> This would become
>
>   class Foo {
>  class Bar {};
>   };
>
> i.e. its going to remove the extra lines, just asking so we can understand if 
> the removal of the line is the error or the fact it doesn't remove the line 
> in the first place?

It is different, the issue I mentioned is about the empty lines in namespace.

As for class, clang-format always removes the empty lines in class:

  darwin@Darwins-iMac temp % cat f.cpp 
  class Foo {
  
  class Bar {} ;
  
  };
  darwin@Darwins-iMac temp % clang-format f.cpp -style="{BasedOnStyle: google, 
BreakBeforeBraces: Custom, BraceWrapping: {AfterClass: true}}" 
  class Foo
  {
class Bar
{
};
  };
  darwin@Darwins-iMac temp % clang-format f.cpp -style="{BasedOnStyle: google, 
BreakBeforeBraces: Custom, BraceWrapping: {AfterClass: false}}"
  class Foo {
class Bar {};
  };

Except for when `KeepEmptyLinesAtTheStartOfBlocks` is true:

  darwin@Darwins-iMac temp % clang-format f.cpp -style="{BasedOnStyle: google, 
BreakBeforeBraces: Custom, BraceWrapping: {AfterClass: false}, 
KeepEmptyLinesAtTheStartOfBlocks: true}"
  class Foo {
  
class Bar {};
  };


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-13 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2813515 , @MyDeveloperDay 
wrote:

> Do we need a set options for when we want to insert/retain/add a newline 
> after various constructs? frankly I've been mulling over the concept of 
> adding a
>
>   NewLinesBetweenFunctions: 1
>
> I personally don't like code written like this as I find it hard to read, I'd 
> like to be able to mandate a single line between functions
>
>   void foo()
>   {
>   ...
>   }
>   void bar()
>   {
>   ...
>   }
>   void foobar()
>   {
>   ...
>   }
>
> I prefer when its written as:
>
>   void foo()
>   {
>   ...
>   }
>   
>   void bar()
>   {
>   ...
>   }
>   
>   void foobar()
>   {
>   ...
>   }
>
> Maybe we even need a more generalised mechanism that would allow alot of 
> flexibility letting people control their own specific style.
>
>   EmptyLineInsertionStyle: Custom
> AfterNameSpaceOpeningBrace: 1
> BeforeNameSpaceClosingBrace: 1
> BetweenFunctions: 2
> AfterClassOpeningBrace: 1
> BeforeClassClosingBrace: 1
> AfterAccessModifier : 1
> BeforeAccessModifier: 1

I totally agree with you on this part, but I think this is a new feature 
requirement. Maybe we can open a new bug and set the "Severity" to 
"enhancement".

In D104044#2813794 , @MyDeveloperDay 
wrote:

> My point being there is inconsistency between how different types of blocks 
> of code are handled, and rather than trying to fix another corner case maybe 
> we should take a more holistic approach, all these KeepEmptyLines and 
> EmptyLineAfterXXX options and what you'll need in order to fix this issue are 
> all addressing what is effectively the same issue, and that is that the 
> addition and/or removal of empty lines is a little hit or miss depending on 
> your combination and permutation of settings and the type of block

I agree that we should use a holistic approach to solve the problem as long as 
we can, but I think the namespace is different than the class based on those 
reasons:

- We indent in the class scope, but we almost never indent in the namespace 
scope. (I've checked all the predefined styles)
- The life-cycles of the objects in the class scope are associated with the 
class scope, but the life-cycles of the objects in a namespace is always global.

> I personally would prefer we took a step back and asked ourselves if we are 
> really facing a bug here or just different people desiring different 
> functionality?

My reason for this being a bug is very simple. If my original code is like this 
(original):

  01 namespace B
  02 {
  03
  04
  04 int j;
  05
  06
  07 }

Then I want the code to be formatted like this (expected):

  01 namespace B
  02 {
  03
  04 int j;
  05
  06 }

Not like this (actual):

  01 namespace B
  02 {
  03 int j;
  04
  05 }

> Whatever the rules are for an inner class, I don't particularly see they are 
> much different for a class in a namespace (which I why I picked that example 
> to demonstrate the point), we won't resolve that inconsistency in a way that 
> will satisfy everyone without having a more powerful mechanism.
>
> If you are thinking you want to just fix your bug then I'd be saying that it 
> SHOULD remove the empty lines (including the one prior to the } // namespace 
> MyLibrary, having said that I'm slightly struggling to understand why
>
>   class Foo {
>   
>   
>   
>   
>   class Bar {};
>   };
>
> isn't conforming to the setting of MaxEmptyLinesToKeep if I set it to 1 where

"it SHOULD remove the empty lines (including the one prior to the } // 
namespace MyLibrary", I don't get what you mean here, can you give me an 
example?

As for the code (original):

  darwin@Darwins-iMac temp % cat f.cpp 
  class Foo {
  
  
  
  
  class Bar {};
  };

It will be formatted into:

  darwin@Darwins-iMac temp % clang-format f.cpp -style="{MaxEmptyLinesToKeep: 
1}"
  class Foo {
  
class Bar {};
  };

I didn't see any problem here.

>   namespace MyLibrary {
>   
>   class Tool {};
>   }  // namespace MyLibrary
>
> is
>
> i.e. set MaxEmptyLinesToKeep  to 0 and
>
> it gets formatted as:
>
>   namespace MyLibrary {
>   class Tool {};
>   }  // namespace MyLibrary

The behavior is correct in this case. Notice the `{` is on the same line as the 
`namespace`. If I set `AfterNamespace` as true, then the behavior is different.

Orignal code, there are two empty lines before and after `class Tool {};`:

  darwin@Darwins-iMac temp % cat e.cpp 
  01 namespace MyLibrary {
  02
  03 
  04 class Tool {};
  05 
  06 
  07 } 

If I format it with `AfterNamespace` as true and `MaxEmptyLinesToKeep` as 0, 
all those empty lines are removed, which is correct:

  darwin@Darwins-iMac temp % clang-format e.cpp -style="{BasedOnStyle: google, 
BreakBeforeBraces: Custom, BraceWrapping: {AfterNamespace: true}, 
MaxEmptyLinesToKeep: 0}"
  01 namespace MyLibrary
  02 {
  03 class Tool {};
  04 }  //

[PATCH] D104044: [clang-format] Fix the issue of no empty line after namespace

2021-06-13 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2814016 , 
@HazardyKnusperkeks wrote:

> Going the full way, to fix the number of empty lines after/before/between 
> elements would be real nice. But even nicer would be if you can state a range.
>
> But I think all those proposed options should not be added in one go.

Yes, adding a new option is a new feature requirement. What I am trying to do 
here is to keep the clang-format behaving reasonably.

> In D104044#2812399 , @darwin wrote:
>
>> About the issue, let me explain it. It isn't bound to the google style or 
>> LLVM style either, since both of them keep the first brace at the same line 
>> of the namespace.
>
> Then I would like to use the LLVM style in the tests, otherwise it suggests 
> that the issue is a result of using google style.

Wouldn't this suggest that this issue is a result of using LLVM style?


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-15 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2816397 , @MyDeveloperDay 
wrote:

> I think we can agree its complicated, how about you take your unit tests and 
> show us what the "code change" looks like to fix the bug
>
> If that gets overly convoluted then perhaps we can bring the idea forward of 
> a more generalised approach.

Thanks, I can do that.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-19 Thread Darwin Xu via Phabricator via cfe-commits 
darwin updated this revision to Diff 353200.
darwin added a comment.

Updated with the fix code.

Look into the code, I am pretty sure this is a bug, it is about the logic of 
the parameter `KeepEmptyLinesAtTheStartOfBlocks`.

When `KeepEmptyLinesAtTheStartOfBlocks` is false, it will remove the empty 
lines at the start of the block, and namespace will be an exception. So the 
empty lines will be kept inside namespace.

The problem is, it can only handle the situation in which the `namespace` and 
the `{` are on the same line. If `BraceWrapping.AfterNamespace` is true, it 
will cause the `namespace` and the `{` to be separated into different lines. 
The original code overlooked this situation:

  // Remove empty lines after "{".
  if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
  PreviousLine->Last->is(tok::l_brace) &&
  !PreviousLine->startsWithNamespace() &&
  !startsExternCBlock(*PreviousLine))
Newlines = 1;

As you can see, it only checks if previous line starts with namespace.

The solution is a bit of tricky, we need to check not only the **previous 
line**, but also the **line before the previous line**. There isn't an easy way 
to get this. So I added a new parameter `PrevPrevLine` to the 
`UnwrappedLineFormatter::formatFirstToken()` function. I have to admit this 
isn't an elegant solution. Let me know if there is a better way to do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104044

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineFormatter.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -262,6 +262,89 @@
"}",
getGoogleStyle()));
 
+  auto CustomStyle = clang::format::getLLVMStyle();
+  CustomStyle.BreakBeforeBraces = clang::format::FormatStyle::BS_Custom;
+  CustomStyle.BraceWrapping.AfterNamespace = true;
+  CustomStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
+  EXPECT_EQ("namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("namespace N\n"
+   "{\n"
+   "\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("/* something */ namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("/* something */ namespace N {\n"
+   "\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("inline namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("inline namespace N\n"
+   "{\n"
+   "\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("/* something */ inline namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("/* something */ inline namespace N\n"
+   "{\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("export namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("export namespace N\n"
+   "{\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("namespace a\n"
+"{\n"
+"namespace b\n"
+"{\n"
+"\n"
+"class AA {};\n"
+"\n"
+"} // namespace b\n"
+"} // namespace a\n",
+format("namespace a\n"
+   "{\n"
+   "namespace b\n"
+   "{\n"
+   "\n"
+   "\n"
+   "class AA {};\n"
+   "\n"
+   "\n"
+   "}\n"
+   "}\n",
+   CustomStyle));
+
   // ...but do keep inlining and removing empty lines for non-block extern "C"
   // functions.
   verifyFormat("extern \"C\" int f() { return 42; }", getGoogleStyle());
Index: clang/lib/Format/UnwrappedLineFormatter.h
===
--- clang/lib/Format/UnwrappedLineFormatter.h
+++ clang/lib/Format/UnwrappedLineFormatter.h
@@ -47,6 +47,7 @@
   /// of the \c UnwrappedLine if there was no structural parsing error.
   void formatFirstToken(const AnnotatedLine &Line,
 const AnnotatedLine *PreviousLine,
+const A

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-21 Thread Darwin Xu via Phabricator via cfe-commits 
darwin marked an inline comment as done.
darwin added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:281
+   CustomStyle));
+  EXPECT_EQ("/* something */ namespace N\n"
+"{\n"

MyDeveloperDay wrote:
> What does
> 
> ```
> namespace N  { /* comment */
> ```
> 
> or 
> 
> ```
> namespace N   /* comment */ {
> ```
> 
> do
It works fine: (with `-style="{BasedOnStyle: google, BreakBeforeBraces: Custom, 
BraceWrapping: {AfterNamespace: true}}"`)
```
namespace A /* comment */ { class B {} }

namespace A {/* comment */ class B {} }

namespace A { /* comment */


 class B {}


  }

namespace A/* comment */ {


 class B {}


  }
```
Is formatted like this:
```
namespace A /* comment */
{
class B {}
}  // namespace A

namespace A
{ /* comment */
class B {}
}  // namespace A

namespace A
{ /* comment */

class B {}

}  // namespace A

namespace A /* comment */
{

class B {}

}  // namespace A
```

I will update the test case too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-21 Thread Darwin Xu via Phabricator via cfe-commits 
darwin marked 2 inline comments as done.
darwin added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:280
+   "}",
+   CustomStyle));
+  EXPECT_EQ("/* something */ namespace N\n"

MyDeveloperDay wrote:
> I'm not sure I understand this.. why is this not
> 
> ```
> namespace N
> {
> int i;
> }
> ```
Because a namespace is different than a class or struct.

Please try this with chang-format:
```
darwin@Darwins-iMac Desktop % cat a.cpp 
namespace N {


int i;


}
darwin@Darwins-iMac Desktop % clang-format a.cpp -style="{BasedOnStyle: google}"
namespace N {

int i;

}
```
As you can see, clang-format removes the extra lines but still keeps one line 
at the beginning and the end of the namespace. This is the original behavior, I 
think it is very reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104044

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

[PATCH] D104900: [clang-format] PR50525 doesn't handle AlignConsecutiveAssignments correctly in some situations

2021-06-26 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

Sorry I haven't had a chance to look at this bug before it has closed. But I do 
have question:

I observed that this code are formatted incorrectly by the same config:

  llvm::Optional CurrentCode = None;
  autoEnv = std::make_unique(Code,
   FileName,
   Ranges,
   FirstStartColumn,
   NextStartColumn,
   LastStartColumn);

Can this solution solve this issue? I am afraid not.

If no, should I open a new bug for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104900

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

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-26 Thread Darwin Xu via Phabricator via cfe-commits 
darwin updated this revision to Diff 354696.
darwin marked an inline comment as done.
darwin added a comment.

Add new test cases according to the comments.


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

https://reviews.llvm.org/D104044

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineFormatter.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -262,6 +262,128 @@
"}",
getGoogleStyle()));
 
+  auto CustomStyle = clang::format::getLLVMStyle();
+  CustomStyle.BreakBeforeBraces = clang::format::FormatStyle::BS_Custom;
+  CustomStyle.BraceWrapping.AfterNamespace = true;
+  CustomStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
+  EXPECT_EQ("namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("namespace N\n"
+   "{\n"
+   "\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("/* something */ namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("/* something */ namespace N {\n"
+   "\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("inline namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("inline namespace N\n"
+   "{\n"
+   "\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("/* something */ inline namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("/* something */ inline namespace N\n"
+   "{\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("export namespace N\n"
+"{\n"
+"\n"
+"int i;\n"
+"}",
+format("export namespace N\n"
+   "{\n"
+   "\n"
+   "inti;\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("namespace a\n"
+"{\n"
+"namespace b\n"
+"{\n"
+"\n"
+"class AA {};\n"
+"\n"
+"} // namespace b\n"
+"} // namespace a\n",
+format("namespace a\n"
+   "{\n"
+   "namespace b\n"
+   "{\n"
+   "\n"
+   "\n"
+   "class AA {};\n"
+   "\n"
+   "\n"
+   "}\n"
+   "}\n",
+   CustomStyle));
+  EXPECT_EQ("namespace A /* comment */\n"
+"{\n"
+"class B {}\n"
+"} // namespace A",
+format("namespace A /* comment */ { class B {} }", CustomStyle));
+  EXPECT_EQ("namespace A\n"
+"{ /* comment */\n"
+"class B {}\n"
+"} // namespace A",
+format("namespace A {/* comment */ class B {} }", CustomStyle));
+  EXPECT_EQ("namespace A\n"
+"{ /* comment */\n"
+"\n"
+"class B {}\n"
+"\n"
+""
+"} // namespace A",
+format("namespace A { /* comment */\n"
+   "\n"
+   "\n"
+   "class B {}\n"
+   "\n"
+   "\n"
+   "}",
+   CustomStyle));
+  EXPECT_EQ("namespace A /* comment */\n"
+"{\n"
+"\n"
+"class B {}\n"
+"\n"
+"} // namespace A",
+format("namespace A/* comment */ {\n"
+   "\n"
+   "\n"
+   "class B {}\n"
+   "\n"
+   "\n"
+   "}",
+   CustomStyle));
+
   // ...but do keep inlining and removing empty lines for non-block extern "C"
   // functions.
   verifyFormat("extern \"C\" int f() { return 42; }", getGoogleStyle());
Index: clang/lib/Format/UnwrappedLineFormatter.h
===
--- clang/lib/Format/UnwrappedLineFormatter.h
+++ clang/lib/Format/UnwrappedLineFormatter.h
@@ -47,6 +47,7 @@
   /// of the \c UnwrappedLine if there was no structural parsing error.
   void formatFirstToken(const AnnotatedLine &Line,
 const AnnotatedLine *PreviousLine,
+const AnnotatedLine *PrevPrevLine,

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-26 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

And can someone commit it for me? I don't have the right to push it yet. Or let 
me know how to apply? Thank you very much.


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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-26 Thread Darwin Xu via Phabricator via cfe-commits 
darwin added a comment.

In D104044#2842757 , 
@HazardyKnusperkeks wrote:

> In D104044#2842726 , @darwin wrote:
>
>> And can someone commit it for me? I don't have the right to push it yet. Or 
>> let me know how to apply? Thank you very much.
>
> https://llvm.org/docs/DeveloperPolicy.html?highlight=chris#obtaining-commit-access

Thanks, guess I still need you to commit it for me (User: Darwin Xu, Email: 
darwin...@icloud.com), I cannot get the access any time soon. Thank you.


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

https://reviews.llvm.org/D104044

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

[PATCH] D104044: [clang-format] Fix the issue that empty lines being removed at the beginning of namespace

2021-06-28 Thread Darwin Xu via Phabricator via cfe-commits 
darwin marked an inline comment as done.
darwin added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1270
   !startsExternCBlock(*PreviousLine))
 Newlines = 1;
 

MyDeveloperDay wrote:
> Nit:I do think at some point we need to have some sort of option for 
> controlling these empty lines, 
> 
> 
> EmptyLine:
>  AfterNamespace: 1
>  BeforeAccessModifier: 1
>  AfterAccessModifier: 1
>  BetweenFunctions: 1
>  ...
> 
Yes, maybe we can open a bug for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104044

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

[PATCH] D97137: [clang-format] Fix AlignConsecutiveDeclarations handling of pointers

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

In D97137#2587075 , 
@HazardyKnusperkeks wrote:

> 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.

Darwin Xu, darwin...@icloud.com. Thank you very much.


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