[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Nikola Smiljanić via Phabricator via cfe-commits]

nikola added a comment.

Commit r298574, thanks for woking on this folks!


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daphne Pfister via Phabricator via cfe-commits]

daphnediane added a comment.

ping


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper added a comment.

Hi @djasper,
This is the first patch I've contributed here, so I'm not familiar with the 
whole process. I assume this code is ready to land? When exactly does it get 
merged into master, and is there something else that I still need to do to make 
that happen?

Thanks,
Ben


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper updated this revision to Diff 89463.
bmharper added a comment.

Fixed two small issues raised by @daphnediane


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7539,12 +7539,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  2) ? 3 : 4);\n"
"float b[1][] = {{3.f}};\n",
Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+   "  int x = 1;\n",
+   Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+   "  x = 1;\n"
+   "y = 1;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
@@ -7626,7 +7632,57 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Function prototype alignment
+  verifyFormat("inta();\n"
+   "double b();",
+   Alignment);
+  verifyFormat("inta(int x);\n"
+   "double b();",
+   Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("inta(int   x,\n"
+" float y);\n"
+"double b(intx,\n"
+" double y);",
+format("int a(int x,\n"
+   " float y);\n"
+   "double b(int x,\n"
+   " double y);",
+   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+"intb(inty,\n"
+" double z);",
+format("double a(int x);\n"
+   "int b(int y,\n"
+   " double z);",
+   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+   "int  x,\n"
+   "bool y);",
+   Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("inta(int x, void (*fp)(int y));\n"
+   "double b();",
+   Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+   "  int b  = 2;\n"
+   "  int cc = 3;\n"
+   "}",
+   Alignment);
   verifyFormat("float  something = 2000;\n"
"double another   = 911;\n"
"inti = 1, j = 10;\n"
@@ -7636,6 +7692,28 @@
   verifyFormat("int  oneTwoThree = {0}; // comment\n"
"unsigned oneTwo  = 0;   // comment",
Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  verifyFormat("if (int i = 0)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator[](int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator()(int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
 "  int const i   = 1;\n"
 "  int * j   = 2;\n"
@@ -7737,17 +7815,16 @@
Alignment);
   Alignment.AlignConsecutiveAssignments = false;
 
-  // FIXME: Should align all three declarations
   verifyFormat(
   "int  i = 1;\n"
  

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daphne Pfister via Phabricator via cfe-commits]

daphnediane added a comment.

Rebuilt with the latest patch and got one compile error. See line comment 
worked okay after fixing it.




Comment at: lib/Format/WhitespaceManager.cpp:215
+
+if (i != Start) {
+  if (Changes[i].nestingAndIndentLevel() >

These ifs can get merged again, when you merged my changes in it was based on a 
version before you merged them.



Comment at: lib/Format/WhitespaceManager.h:157
+std::pair
+WhitespaceManager::Change::nestingAndIndentLevel() const {
+  return std::make_pair(Tok->NestingLevel, Tok->IndentLevel);

Extra WhitespaceManager::Change:: prefix here


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper added a comment.

Thanks for all the code review work! I'm impressed by the quality standard 
maintained here.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper updated this revision to Diff 87478.
bmharper added a comment.

small comment tweak


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9404,12 +9404,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  2) ? 3 : 4);\n"
"float b[1][] = {{3.f}};\n",
Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+   "  int x = 1;\n",
+   Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+   "  x = 1;\n"
+   "y = 1;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
@@ -9491,7 +9497,57 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Function prototype alignment
+  verifyFormat("inta();\n"
+   "double b();",
+   Alignment);
+  verifyFormat("inta(int x);\n"
+   "double b();",
+   Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("inta(int   x,\n"
+" float y);\n"
+"double b(intx,\n"
+" double y);",
+format("int a(int x,\n"
+   " float y);\n"
+   "double b(int x,\n"
+   " double y);",
+   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+"intb(inty,\n"
+" double z);",
+format("double a(int x);\n"
+   "int b(int y,\n"
+   " double z);",
+   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+   "int  x,\n"
+   "bool y);",
+   Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("inta(int x, void (*fp)(int y));\n"
+   "double b();",
+   Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+   "  int b  = 2;\n"
+   "  int cc = 3;\n"
+   "}",
+   Alignment);
   verifyFormat("float  something = 2000;\n"
"double another   = 911;\n"
"inti = 1, j = 10;\n"
@@ -9501,6 +9557,28 @@
   verifyFormat("int  oneTwoThree = {0}; // comment\n"
"unsigned oneTwo  = 0;   // comment",
Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  verifyFormat("if (int i = 0)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator[](int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator()(int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
 "  int const i   = 1;\n"
 "  int * j   = 2;\n"
@@ -9602,17 +9680,16 @@
Alignment);
   Alignment.AlignConsecutiveAssignments = false;
 
-  // FIXME: Should align all three declarations
   verifyFormat(
   "int  i = 1;\n"
   "SomeType a = 

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

This looks very nice now :-D. Thanks for working on this!!




Comment at: lib/Format/WhitespaceManager.cpp:196
+
+  // ScopeStack keeps track of the current scope depth.
+  // We only run the "Matches" function on tokens from the outer-most scope.

Maybe add: "It contains indices of the first token on each scope."


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper updated this revision to Diff 87361.
bmharper added a comment.

This looks a lot better IMO. Thanks @daphnediane - you should recognize the 
code ;)
The special closing paren logic inside of nestingAndIndentLevel() is indeed 
redundant now.


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9404,12 +9404,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  2) ? 3 : 4);\n"
"float b[1][] = {{3.f}};\n",
Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+   "  int x = 1;\n",
+   Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+   "  x = 1;\n"
+   "y = 1;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
@@ -9491,7 +9497,57 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Function prototype alignment
+  verifyFormat("inta();\n"
+   "double b();",
+   Alignment);
+  verifyFormat("inta(int x);\n"
+   "double b();",
+   Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("inta(int   x,\n"
+" float y);\n"
+"double b(intx,\n"
+" double y);",
+format("int a(int x,\n"
+   " float y);\n"
+   "double b(int x,\n"
+   " double y);",
+   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+"intb(inty,\n"
+" double z);",
+format("double a(int x);\n"
+   "int b(int y,\n"
+   " double z);",
+   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+   "int  x,\n"
+   "bool y);",
+   Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("inta(int x, void (*fp)(int y));\n"
+   "double b();",
+   Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+   "  int b  = 2;\n"
+   "  int cc = 3;\n"
+   "}",
+   Alignment);
   verifyFormat("float  something = 2000;\n"
"double another   = 911;\n"
"inti = 1, j = 10;\n"
@@ -9501,6 +9557,28 @@
   verifyFormat("int  oneTwoThree = {0}; // comment\n"
"unsigned oneTwo  = 0;   // comment",
Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  verifyFormat("if (int i = 0)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator[](int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator()(int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
 "  int const i   = 1;\n"
 "  int * j   = 2;\n"
@@ -9602,17 +9680,16 @@
Alignment);
   

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daphne Pfister via Phabricator via cfe-commits]

daphnediane added a comment.

In https://reviews.llvm.org/D21279#667471, @bmharper wrote:

> Thanks @daphnediane. I only read your comment after merging, but that would 
> have been helpful.


If you still want my diff let me know as it is slightly different from yours. 
No longer has NestingAndIndent level as a data member of Changes ( but has a 
inline method that gets the values though not 100% sure that is needed as I 
experimented with a version without it), no longer needs propagateIndentLevels, 
etc.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:190
 
+struct TokenTypeAndLevel {
+  TokenType Type;

I don't think you need this struct now. Just use the FormatToken itself, it 
should have all of this information.



Comment at: lib/Format/WhitespaceManager.cpp:195
+
+// Upon entry, IndentLevel is only set on the first token of the line.
+// Here we propagate IndentLevel to every token on the line.

This is no longer true. IndentLevel should be set correctly for every token.



Comment at: lib/Format/WhitespaceManager.cpp:288
+// If this function encounters a scope level less than the initial level,
+// it exits.
+// There is a non-obvious subtlety in the recursive behavior: Even though we

nit: s/exits/returns/

(or maybe even "returns the current position")



Comment at: lib/Format/WhitespaceManager.cpp:359
   ++CommasBeforeMatch;
-} else if (Changes[i].Tok->isOneOf(tok::r_brace, tok::r_paren,
-   tok::r_square)) {
-  --NestingLevel;
-} else if (Changes[i].Tok->isOneOf(tok::l_brace, tok::l_paren,
-   tok::l_square)) {
-  // We want sequences to skip over child scopes if possible, but not the
-  // other way around.
-  NestingLevelOfLastMatch = std::min(NestingLevelOfLastMatch, 
NestingLevel);
-  ++NestingLevel;
+} else if (Changes[i].NestingAndIndentLevel != NestingAndIndentLevel) {
+  // Call AlignTokens recursively, skipping over this scope block.

The "!=" is a bit confusing here. ">" would do the same thing, right (because 
"<" is already handled above)?



Comment at: lib/Format/WhitespaceManager.h:130
+// used to add tabs for indentation.
+std::pair NestingAndIndentLevel;
+

I think you can get rid of this field now and just directly use the values 
stored in the tokens.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper added a comment.

Thanks @daphnediane. I only read your comment after merging, but that would 
have been helpful.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper updated this revision to Diff 87179.
bmharper added a comment.

Fixed a stale comment


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9404,12 +9404,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  2) ? 3 : 4);\n"
"float b[1][] = {{3.f}};\n",
Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+   "  int x = 1;\n",
+   Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+   "  x = 1;\n"
+   "y = 1;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
@@ -9491,7 +9497,57 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Function prototype alignment
+  verifyFormat("inta();\n"
+   "double b();",
+   Alignment);
+  verifyFormat("inta(int x);\n"
+   "double b();",
+   Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("inta(int   x,\n"
+" float y);\n"
+"double b(intx,\n"
+" double y);",
+format("int a(int x,\n"
+   " float y);\n"
+   "double b(int x,\n"
+   " double y);",
+   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+"intb(inty,\n"
+" double z);",
+format("double a(int x);\n"
+   "int b(int y,\n"
+   " double z);",
+   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+   "int  x,\n"
+   "bool y);",
+   Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("inta(int x, void (*fp)(int y));\n"
+   "double b();",
+   Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+   "  int b  = 2;\n"
+   "  int cc = 3;\n"
+   "}",
+   Alignment);
   verifyFormat("float  something = 2000;\n"
"double another   = 911;\n"
"inti = 1, j = 10;\n"
@@ -9501,6 +9557,28 @@
   verifyFormat("int  oneTwoThree = {0}; // comment\n"
"unsigned oneTwo  = 0;   // comment",
Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  verifyFormat("if (int i = 0)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator[](int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator()(int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
 "  int const i   = 1;\n"
 "  int * j   = 2;\n"
@@ -9602,17 +9680,16 @@
Alignment);
   Alignment.AlignConsecutiveAssignments = false;
 
-  // FIXME: Should align all three declarations
   verifyFormat(
   "int  i = 1;\n"
   "SomeType a = 

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper updated this revision to Diff 87178.
bmharper added a comment.

Hi @djasper,
I've made the latest bunch of review changes, and rebased onto master. I didn't 
check the numbers, but I think the code is slightly smaller after the rebase.

Regards,
Ben


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9404,12 +9404,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  2) ? 3 : 4);\n"
"float b[1][] = {{3.f}};\n",
Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+   "  int x = 1;\n",
+   Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+   "  x = 1;\n"
+   "y = 1;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
@@ -9491,7 +9497,57 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Function prototype alignment
+  verifyFormat("inta();\n"
+   "double b();",
+   Alignment);
+  verifyFormat("inta(int x);\n"
+   "double b();",
+   Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("inta(int   x,\n"
+" float y);\n"
+"double b(intx,\n"
+" double y);",
+format("int a(int x,\n"
+   " float y);\n"
+   "double b(int x,\n"
+   " double y);",
+   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+"intb(inty,\n"
+" double z);",
+format("double a(int x);\n"
+   "int b(int y,\n"
+   " double z);",
+   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+   "int  x,\n"
+   "bool y);",
+   Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("inta(int x, void (*fp)(int y));\n"
+   "double b();",
+   Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+   "  int b  = 2;\n"
+   "  int cc = 3;\n"
+   "}",
+   Alignment);
   verifyFormat("float  something = 2000;\n"
"double another   = 911;\n"
"inti = 1, j = 10;\n"
@@ -9501,6 +9557,28 @@
   verifyFormat("int  oneTwoThree = {0}; // comment\n"
"unsigned oneTwo  = 0;   // comment",
Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  verifyFormat("if (int i = 0)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator[](int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator()(int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
 "  int const i   = 1;\n"
 "  int * j   = 2;\n"
@@ -9602,17 +9680,16 @@
Alignment);
   

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daphne Pfister via Phabricator via cfe-commits]

daphnediane added a comment.

In https://reviews.llvm.org/D21279#663670, @bmharper wrote:

> Thanks - the merge conflicts don't look too bad. I should have it cleaned up 
> by tomorrow.


Have you had a chance to do the merge yet? If not I have a merged version which 
passes the tests that I could post here if you want.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper added a comment.

Thanks - the merge conflicts don't look too bad. I should have it cleaned up by 
tomorrow.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I apologize in advance if this causes merge conflicts with r293616. However, I 
do hope that that actually makes this patch easier.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:207
+
+if (i != Start) {
+  if (Changes[i].NestingAndIndentLevel >

Merge the two ifs into a single one?



Comment at: lib/Format/WhitespaceManager.cpp:318
+  for (unsigned e = Changes.size();
+   i != e && Changes[i].NestingAndIndentLevel >= NestingAndIndentLevel;
+   ++i) {

I'd probably move the second condition into an early exit inside the loop:

  if (Changes[i].NestingAndIndentLevel >= NestingAndIndentLevel)
break;




Comment at: lib/Format/WhitespaceManager.cpp:394
   },
-  Changes);
+  Changes, 0);
 }

Use a comment to describe the literal, i.e.:

  /*StartAt=*/0




Comment at: lib/Format/WhitespaceManager.h:134
+// A combination of nesting level and indent level.
+// The nesting level of this token, i.e. the number of surrounding (),
+// [], {} or <>. Note that the value stored here is slightly different

This comment seems outdated.



Comment at: lib/Format/WhitespaceManager.h:217
+  /// declaration name.
+  static bool IsStartOfDeclName(const FormatToken );
+

This function should be a local function in the .cpp file. Also, can you 
describe in more detail what this does, i.e. what kind of declarations are 
covered here? Also, it is a bit confusing to have a function and a member of 
WhitespaceManager::Change with the exact same name.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper added a comment.

Pinging @djasper. Any chance we can get this merged?


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Erik Nyquist via Phabricator via cfe-commits]

enyquist added a comment.

Well, your patch is here for me to try, and it looks like it's been accepted. 
So I guess I should just pull my finger out and try it :)
Thanks for your response-- I'll let you know if I come across any issues.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper added a comment.

Hi @enyquist,
I'd like to guess that this patch will solve your problem, but I'm not intimate 
enough with this code to give you a definitive answer. I hope we can get this 
merged soon, so that you can just run it and see.

Ben


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Erik Nyquist via Phabricator via cfe-commits]

enyquist added a comment.

Hey bmharper :) I've got a review open that conflicts with this one, just 
having a look to see what I'll need to refactor
(https://reviews.llvm.org/D28462).

In fact, I have a question-- the conflict is specifically in 
WhitespaceManager.cpp. Since I needed to detect PP macros containing changes in 
scope depth (code blocks surrounded by curly braces, macro parameter lists, 
etc), I was having the same problem as you-- AlignTokens was bailing out 
whenever the scope depth changed.

In my case, I just added a new parameter to AlignTokens, 
MaxNestingLevelIncrease, indicating how much the level can increase before we 
stop alignment, making the allowable scope-depth configurable. For example, 
calling AlignTokens with this flag set to 2 will cause alignment to continue up 
until we increase scope by two levels.

Now, my question- from what I can tell of your changes, it looks like my code 
can actually be simpler when this gets merged. The state of AlignTokens will be 
maintained across changing scope depths, and I won't need to modify AlignTokens 
so that it can survive something like "#define foo(x) ((x * 2) + 2)". Is  this 
correct?


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via Phabricator via cfe-commits]

berenm added a comment.

Pinging @djasper

There's https://reviews.llvm.org/D27651 that will conflict with this one.


https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via Phabricator via cfe-commits]

bmharper updated this revision to Diff 81213.
bmharper added a comment.

Sorry for the incredibly long turnaround time on this one - I don't have any 
legitimate excuse, and I know it just makes reviewing it harder.

Anyway - I have sorted out all the issues mentioned in the last review. One 
thing that you suggested was to go ahead and replace the std::pair for 
NestingAndIndentLevel, which I did write, but I feel like it adds quite a bit 
of bloat, because you need two constructors, operator==, operator!=, operator<, 
operator>, so I'm not convinced that it's worth it in the end. If you still 
think it is, then I'll go ahead and add it.

Regards,
Ben


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8612,12 +8612,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  2) ? 3 : 4);\n"
"float b[1][] = {{3.f}};\n",
Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+   "  int x = 1;\n",
+   Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+   "  x = 1;\n"
+   "y = 1;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
@@ -8699,7 +8705,57 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Function prototype alignment
+  verifyFormat("inta();\n"
+   "double b();",
+   Alignment);
+  verifyFormat("inta(int x);\n"
+   "double b();",
+   Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("inta(int   x,\n"
+" float y);\n"
+"double b(intx,\n"
+" double y);",
+format("int a(int x,\n"
+   " float y);\n"
+   "double b(int x,\n"
+   " double y);",
+   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+"intb(inty,\n"
+" double z);",
+format("double a(int x);\n"
+   "int b(int y,\n"
+   " double z);",
+   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+   "int  x,\n"
+   "bool y);",
+   Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("inta(int x, void (*fp)(int y));\n"
+   "double b();",
+   Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+   "  int b  = 2;\n"
+   "  int cc = 3;\n"
+   "}",
+   Alignment);
   verifyFormat("float  something = 2000;\n"
"double another   = 911;\n"
"inti = 1, j = 10;\n"
@@ -8709,6 +8765,28 @@
   verifyFormat("int  oneTwoThree = {0}; // comment\n"
"unsigned oneTwo  = 0;   // comment",
Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  verifyFormat("if (int i = 0)\n"
+   "  j = i;\n"
+   "double x = 1;\n",
+   Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+   "  long long int foo();\n"
+   "  int   operator[](int a);\n"
+   "  doublebar();\n"
+   "};\n",
+   Alignment);
+  

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

So sorry. Seems I forgot to hit "Submit" :(.

If you don't like the ".first" and ".second" of the pair, you could introduce a 
struct for it and overload operator<. Might actually be more readable.



> WhitespaceManager.cpp:73
> +  Tok.NestingLevel,
>/*Spaces=*/0, Tok.OriginalColumn, Tok.NewlinesBefore, "", "",
> +  Tok.Tok.getKind(), Tok.Type, InPPDirective && !Tok.IsFirst,

nit: Move these to the previous line. clang-format won't do that, because of 
the comment, but that's actually irrelevant here.

> WhitespaceManager.cpp:183
> +
> +// NestingLevel is raised on the opening paren/square, and remains raised
> +// until AFTER the closing paren/square. This means that with a statement

I don't see why this would be necessary. If I remove it, all tests do still 
pass.

> WhitespaceManager.cpp:190
> +//
> +// The "int x = 1" line is going to have the same NestingLevel as
> +// the tokens inside the parentheses of the "for" statement.

Also, this comment seems wrong? The "int x = 1;" actually starts a new (child) 
line. If that has the same nesting level, that seems like a bug we need to fix.

> WhitespaceManager.cpp:210
> +  // We only run the "Matches" function on tokens from the outer-most scope.
> +  // However, we do need to adjust some special tokens within the entire
> +  // Start..End range, regardless of their scope. This special rule applies

Make this (and maybe a few others) more concrete. Don't write "some special 
tokens", write what they actually are.

> WhitespaceManager.cpp:214
> +  // who's function names have been shifted for declaration alignment's sake.
> +  // See "split function parameter alignment" in FormatTest.cpp for an 
> example.
> +  SmallVector ScopeStack;

If the example isn't too long, writing the source code in the comment seems 
better than referencing the test.

> WhitespaceManager.cpp:389
>  
> -  EndOfSequence = Changes.size();
> +  unsigned StoppedAt = i;
> +  EndOfSequence = i;

Either do:

  EndOfSequence = StoppedAt;

or just remove StoppedAt and use i.

> FormatTest.cpp:9364
> +  // WhitespaceManager.cpp
> +  EXPECT_EQ("double a(int x);\n"
> +"intb(intx,\n"

Can you add a test case where there is a line wrap after the "("?

https://reviews.llvm.org/D21279



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

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

ping!


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper updated this revision to Diff 72444.
bmharper added a comment.

This revision merges NestingLevel and IndentLevel into an std::pair, as 
suggested by @djasper.
IMO this makes the code slightly harder to read, because you lose the unique 
variable names "IndentLevel" and "NestingLevel". Those are now replaced by 
.first and .second, respectively. In addition, NestingLevel is no longer equal 
to the original NestingLevel that gets passed in, because it needs to be 
tweaked slightly to better work with the recursive technique we use for 
alignment scope.
However, the approach presented here certainly does work, and it's actually not 
too much of a code change from the previous patch.


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9250,12 +9250,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template . Note that the value stored here is slightly different
+// to the NestingLevel passed in. See propagateIndentLevels() for details.
+// The second element in the pair, the indent level, is used to add tabs
 // only for the indentation, and not for alignment, when
 // UseTab = US_ForIndentation.
-unsigned IndentLevel;
+std::pair NestingAndIndentLevel;
 
 // The number of spaces in front of the token or broken part of the token.
 // This will be adapted when aligning tokens.
@@ -170,6 +176,10 @@
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
+  /// \brief Propagate IndentLevel from the first token on a line, to all of
+  /// the tokens on that line.
+  void propagateIndentLevels();
+
   /// \brief Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
@@ -202,6 +212,10 @@
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
+  /// \brief Determine whether this token is considered as the start of a
+  /// declaration name.
+  static bool IsStartOfDeclName(const FormatToken );
+
   SmallVector Changes;
   const SourceManager 
   tooling::Replacements Replaces;
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -27,19 +27,21 @@
 
 WhitespaceManager::Change::Change(
 bool CreateReplacement, SourceRange OriginalWhitespaceRange,
-unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
-unsigned NewlinesBefore, StringRef PreviousLinePostfix,
-StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective,
+unsigned IndentLevel, unsigned NestingLevel, int Spaces,
+unsigned StartOfTokenColumn, unsigned NewlinesBefore,
+StringRef PreviousLinePostfix, StringRef CurrentLinePrefix,
+tok::TokenKind Kind, TokenType Type, bool ContinuesPPDirective,
 bool IsStartOfDeclName, bool IsInsideToken)
 : CreateReplacement(CreateReplacement),
   OriginalWhitespaceRange(OriginalWhitespaceRange),
   StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
   PreviousLinePostfix(PreviousLinePostfix),
-  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind),
+  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind), Type(Type),
   ContinuesPPDirective(ContinuesPPDirective),
-  IsStartOfDeclName(IsStartOfDeclName), IndentLevel(IndentLevel),
-  Spaces(Spaces), IsInsideToken(IsInsideToken), IsTrailingComment(false),
-  TokenLength(0), PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
+  IsStartOfDeclName(IsStartOfDeclName),
+  NestingAndIndentLevel(NestingLevel, IndentLevel), Spaces(Spaces),
+  IsInsideToken(IsInsideToken), IsTrailingComment(false), TokenLength(0),
+  PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
   StartOfBlockComment(nullptr), IndentationOffset(0) {}
 
 void WhitespaceManager::reset() {
@@ -56,22 +58,21 @@
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
   Changes.push_back(
   Change(/*CreateReplacement=*/true, Tok.WhitespaceRange, IndentLevel,
- Spaces, StartOfTokenColumn, Newlines, "", "", Tok.Tok.getKind(),
- InPPDirective && !Tok.IsFirst,
- Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
- /*IsInsideToken=*/false));
+ 

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

In https://reviews.llvm.org/D21279#550565, @djasper wrote:

> Are we talking completely past each other? I specifically think we should 
> *NOT* combine NestingLevel and IndentLevel into one value. Not in 
> ScopeLevel() and not anywhere else.


Ok, I probably misunderstood the situation, sorry.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

Are we talking completely past each other? I specifically think we should *NOT* 
combine NestingLevel and IndentLevel into one value. Not in ScopeLevel() and 
not anywhere else.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

Hello,

I had a little bit of look into the NestingLevel field. I understand that it 
only indicates the nesting level of the token inside the current unwrapped 
line, which could very well be the same as the nesting level of another token 
in the previous or next unwrapped line. Right now, it doesn't include the 
information on the nested level of the unwrapped line itself (this is in the 
Level field of the line itself).

I'm not sure if the value of IndentLevel comes directly from the unwrapped 
line's Level, but I believe that combining them would give an equivalent of a 
"global" nesting level of the token.

In order to be cleaner, I think it could be done in the AnnotatedLineParser 
class, that already fills the NestingLevel field. I wrote a patch that 
demonstrates this here: https://reviews.llvm.org/D24859. All the unit tests are 
passing, although I'm not 100% sure about all the implications this changes 
has. With this patch, I believed NestingLevel can be used directly to determine 
begin and end of alignment sequences, without requiring the ScopeLevel helper 
function.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

So.. I finally got some time to look at this again:

Quick Recap - IndentLevel and NestingLevel are now stored separately inside 
WhitespaceManager::Change. I've added a function ScopeLevel() which combines 
them with a bit of logic, and returns a number that can be used for alignment 
scope detection purposes.
Now the reason why I don't want to combine IndentLevel and NestingLevel into 
one value:

IndentLevel is used by a function called WhitespaceManager::appendIndentText. I 
don't understand exactly what this function is doing, and despite some 
attempts, I haven't managed to craft an input which gets it to run down the 
code paths I'm interested in. Now as far as I can tell from the experiments 
I've been able to come up with, it's OK to combine IndentLevel and NestingLevel 
into a single number, because it has no observable effect on appendIndentText. 
HOWEVER, just because I can't reproduce the conditions necessary for that code 
to run, doesn't mean there isn't some body of code out there that does stress 
those code paths. It seems like a reasonable approach to me to maintain the 
separate variables IndentLevel and NestingLevel, primarily because those 
keywords are searchable in the rest of the code, and it's easy to trace their 
lineage back to the places where they're generated. If we were to combine them, 
and appendIndentText happens to be broken by this change, then it's going to be 
very confusing for the next guy to come and figure out why this is so. At 
present, IndentLevel means what it says, and so does NestingLevel, and I 
believe, so does ScopeLevel, so I think it's a better idea to keep them 
separate.

Ben


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added inline comments.


Comment at: lib/Format/WhitespaceManager.cpp:47
@@ +46,3 @@
+
+int WhitespaceManager::Change::ScopeLevel() const {
+  // NestingLevel is raised on the opening paren/square, and remains raised

What I don't understand is why you have to combine NestingLevel and IndentLevel 
at all. To me it feels wrong to add them no matter what (with and without your 
extra bit of logic).

IMO, for alignment, we should ensure that both NestingLevel *and* IndentLevel 
are the same, not just the the sum of the two is the same. That's why I was 
suggesting putting them into a pair and just comparing the pair. But I might be 
missing something very obvious.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

PING!
My previous commit hopefully addressed the issue with the sprawl of IndentLevel 
+ ScopeLevel


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper updated this revision to Diff 69671.
bmharper added a comment.

I have added an initial phase which propagates IndentLevel from the first token 
on a line, to all of the tokens on that line. This allows us to get rid of the 
ScopeLevel state. However, I have retained the name "ScopeLevel", and made it a 
member function of Change. I think this is better than putting IndentLevel and 
NestingLevel inside an std::pair, because by retaining the words IndentLevel 
and NestingLevel, it's easy to navigate the rest of the source code and 
discover where those values come from. Additionally, the function ScopeLevel() 
needs to execute one tiny, but vital piece of logic, in order to be useful for 
alignment purposes. One cannot simply add IndentLevel and NestingLevel 
together. This is explained in detail inside the body of the ScopeLevel() 
function.


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9250,12 +9250,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template .
+unsigned NestingLevel;
+
 // The number of spaces in front of the token or broken part of the token.
 // This will be adapted when aligning tokens.
 // Can be negative to retain information about the initial relative offset
@@ -170,6 +180,10 @@
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
+  /// \brief Propagate IndentLevel from the first token on a line, to all of
+  /// the tokens on that line.
+  void propagateIndentLevels();
+
   /// \brief Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
@@ -202,6 +216,10 @@
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
+  /// \brief Determine whether this token is considered as the start of a
+  /// declaration name.
+  static bool IsStartOfDeclName(const FormatToken );
+
   SmallVector Changes;
   const SourceManager 
   tooling::Replacements Replaces;
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -27,20 +27,41 @@
 
 WhitespaceManager::Change::Change(
 bool CreateReplacement, SourceRange OriginalWhitespaceRange,
-unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
-unsigned NewlinesBefore, StringRef PreviousLinePostfix,
-StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective,
+unsigned IndentLevel, unsigned NestingLevel, int Spaces,
+unsigned StartOfTokenColumn, unsigned NewlinesBefore,
+StringRef PreviousLinePostfix, StringRef CurrentLinePrefix,
+tok::TokenKind Kind, TokenType Type, bool ContinuesPPDirective,
 bool IsStartOfDeclName, bool IsInsideToken)
 : CreateReplacement(CreateReplacement),
   OriginalWhitespaceRange(OriginalWhitespaceRange),
   StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
   PreviousLinePostfix(PreviousLinePostfix),
-  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind),
+  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind), Type(Type),
   ContinuesPPDirective(ContinuesPPDirective),
   IsStartOfDeclName(IsStartOfDeclName), IndentLevel(IndentLevel),
-  Spaces(Spaces), IsInsideToken(IsInsideToken), IsTrailingComment(false),
-  TokenLength(0), PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
-  StartOfBlockComment(nullptr), IndentationOffset(0) {}
+  NestingLevel(NestingLevel), Spaces(Spaces), IsInsideToken(IsInsideToken),
+  IsTrailingComment(false), TokenLength(0), PreviousEndOfTokenColumn(0),
+  EscapedNewlineColumn(0), StartOfBlockComment(nullptr),
+  IndentationOffset(0) {}
+
+int WhitespaceManager::Change::ScopeLevel() const {
+  // NestingLevel is raised on the opening paren/square, and remains raised
+  // until AFTER the closing paren/square. This means that with a statement
+  // like:
+  //
+  // for (int i = 0; ...)
+  //   int x = 1;.
+  //
+  // The "int x = 1" line is going to have the same NestingLevel as
+  // the tokens inside the parentheses of the "for" statement.
+  // In order to break this continuity, we force the closing paren/square to
+  // reduce it's nesting level.
+  // If we don't do this, then "x = 1" ends up getting 

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

I'll see what happens if I make IndentLevel the same for all tokens on a line. 
If not too much is broken by that, I should be able to handle it.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

I think the IndentLevel in WhitespaceManager (and the nested Change) is a 
horrible mess and should be cleaned up. It gets set either to 0 or to the 
"Level" of the AnnotatedLine. To me only the latter makes sense as the line 
defines the indent level. Everything else, including recomputing this and 
introducing a ScopeLevel makes the current situation worse. I can try to do 
this in advance of this patch, if you prefer.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

The reason one has to precompute ScopeLevel is because IndentLevel is not 
actually meaningful on each token. It's only meaningful for the first token on 
the line (the remaining tokens on the line have IndentLevel = 0). So if you 
look at the implementation of calculateScopeLevel(), you'll see that the 
function "remembers" the most recent meaningful IndentLevel, and copies that 
into ScopeLevel for all subsequent tokens on the same line.
Now, one could argue that IndentLevel ought to be the same for all tokens on a 
line, but that seems to me like a much bigger change, that would propagate into 
many other parts of the code.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

I think instead of doing some complex computation with LineLevel and 
NestingLevel, it might be better to just leave them as the pair and compare 
them as a pair. The LineLevel should probably always trump the NestingLevel. 
So, I'd try to just defined ScopeLevel as a pair, put both the 
LineLevel and the NestingLevel in it and use that for the comparisons. I might 
be overlooking something, though.


https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper removed rL LLVM as the repository for this revision.
bmharper updated this revision to Diff 68675.
bmharper added a comment.

Fixed the two issues pointed out by djasper in his most recent comments:

1. Only calculate ScopeLevel when necessary.
2. Instead of calculating ScopeLevel by inspecting {[(< and their closing 
pairs, calculate it by combining IndentLevel and Nesting Level. It's not quite 
as simple as just making ScopeLevel = IndentLevel + NestingLevel, but at least 
we don't have yet another function computing scope depth from scratch.


https://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9250,12 +9250,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template .
+unsigned NestingLevel;
+
 // The number of spaces in front of the token or broken part of the token.
 // This will be adapted when aligning tokens.
 // Can be negative to retain information about the initial relative offset
@@ -162,14 +168,22 @@
 // the alignment process.
 const Change *StartOfBlockComment;
 int IndentationOffset;
+
+// This is formed by combining IndentLevel and NestingLevel, in order to
+// produce a scope depth number that is useful for performing consecutive
+// alignment of declarations and assignments.
+int ScopeLevel;
   };
 
 private:
   /// \brief Calculate \c IsTrailingComment, \c TokenLength for the last tokens
   /// or token parts in a line and \c PreviousEndOfTokenColumn and
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
+  /// \brief Calculate \c ScopeLevel for all \c Changes.
+  void calculateScopeLevel();
+
   /// \brief Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
@@ -202,6 +216,10 @@
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
+  /// \brief Determine whether this token is considered as the start of a
+  /// declaration name.
+  static bool IsStartOfDeclName(const FormatToken );
+
   SmallVector Changes;
   const SourceManager 
   tooling::Replacements Replaces;
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -27,20 +27,22 @@
 
 WhitespaceManager::Change::Change(
 bool CreateReplacement, SourceRange OriginalWhitespaceRange,
-unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
-unsigned NewlinesBefore, StringRef PreviousLinePostfix,
-StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective,
+unsigned IndentLevel, unsigned NestingLevel, int Spaces,
+unsigned StartOfTokenColumn, unsigned NewlinesBefore,
+StringRef PreviousLinePostfix, StringRef CurrentLinePrefix,
+tok::TokenKind Kind, TokenType Type, bool ContinuesPPDirective,
 bool IsStartOfDeclName, bool IsInsideToken)
 : CreateReplacement(CreateReplacement),
   OriginalWhitespaceRange(OriginalWhitespaceRange),
   StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
   PreviousLinePostfix(PreviousLinePostfix),
-  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind),
+  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind), Type(Type),
   ContinuesPPDirective(ContinuesPPDirective),
   IsStartOfDeclName(IsStartOfDeclName), IndentLevel(IndentLevel),
-  Spaces(Spaces), IsInsideToken(IsInsideToken), IsTrailingComment(false),
-  TokenLength(0), PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
-  StartOfBlockComment(nullptr), IndentationOffset(0) {}
+  NestingLevel(NestingLevel), Spaces(Spaces), IsInsideToken(IsInsideToken),
+  IsTrailingComment(false), TokenLength(0), PreviousEndOfTokenColumn(0),
+  EscapedNewlineColumn(0), StartOfBlockComment(nullptr),
+  IndentationOffset(0), ScopeLevel(0) {}
 
 void WhitespaceManager::reset() {
   Changes.clear();
@@ -56,22 +58,21 @@
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
   Changes.push_back(
   Change(/*CreateReplacement=*/true, Tok.WhitespaceRange, IndentLevel,
- Spaces, StartOfTokenColumn, Newlines, "", "", Tok.Tok.getKind(),
- InPPDirective && !Tok.IsFirst,
- Tok.is(TT_StartOfName) || 

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

Sorry :(... Review is easy enough.. Feel free to ping me more often in the 
future.



Comment at: lib/Format/WhitespaceManager.cpp:95
@@ -97,2 +94,3 @@
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
+  calculateScopeLevel();
   calculateLineBreakInformation();

bmharper wrote:
> berenm wrote:
> > Maybe we could spare the computation if we aren't going to align anything?
> > 
> > Is it better for clarity to always compute additional information? @djasper 
> > what's the Clang way to do?
> > 
> That's a good point. One certainly could elide that if alignment was turned 
> off. I think so long as it was mentioned in the comments of the ScopeLevel 
> member variable, it would be OK to do so. However, I'll also just defer this 
> decision to @djasper.
Yeah, just avoid unnecessary work.


Comment at: lib/Format/WhitespaceManager.h:173
@@ -165,1 +172,3 @@
+// down to -1.
+int ScopeLevel;
   };

NestingLevel does include braces, generally. However, there are two types:
- Braced initializers: These should just work as is.
- Braces that open blocks: Here, child lines are created and so the tokens 
within a block restart from NestingLevel 0. However, taking that NestingLevel 
in combination with the Level of the AnnotatedLine should work.

I think reusing that is highly preferable over implementing yet another 
parentheses counting.


Repository:
  rL LLVM

https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

PING.

Is there anything I can do to make reviewing easier?

For example, I could run my code vs master on some large code bases (eg
linux kernel, chromium), and verify that there are no crashes, and also
manually spot check some results.

Ben

On Wed, 20 Jul 2016 at 21:43 Ben Harper  wrote:

> bmharper added a comment.
>
> PING
>
>
> 
> Comment at: lib/Format/WhitespaceManager.cpp:95
> @@ -97,2 +94,3 @@
>std::sort(Changes.begin(), Changes.end(),
> Change::IsBeforeInFile(SourceMgr));
> +  calculateScopeLevel();
>calculateLineBreakInformation();
> 
> berenm wrote:
> > Maybe we could spare the computation if we aren't going to align
> anything?
> >
> > Is it better for clarity to always compute additional information?
> @djasper what's the Clang way to do?
> >
> That's a good point. One certainly could elide that if alignment was
> turned off. I think so long as it was mentioned in the comments of the
> ScopeLevel member variable, it would be OK to do so. However, I'll also
> just defer this decision to @djasper.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D21279
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

PING



Comment at: lib/Format/WhitespaceManager.cpp:95
@@ -97,2 +94,3 @@
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
+  calculateScopeLevel();
   calculateLineBreakInformation();

berenm wrote:
> Maybe we could spare the computation if we aren't going to align anything?
> 
> Is it better for clarity to always compute additional information? @djasper 
> what's the Clang way to do?
> 
That's a good point. One certainly could elide that if alignment was turned 
off. I think so long as it was mentioned in the comments of the ScopeLevel 
member variable, it would be OK to do so. However, I'll also just defer this 
decision to @djasper.


Repository:
  rL LLVM

https://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

kaPING!


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

Sorry.. Really busy at the moment, but will try to get this reviewed and 
submitted this week. If not, please ping again!


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

Friendly PING.

Please let me know if there's anything else that I need to do here,
otherwise I'll keep quiet and expect a merge at some point?


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

In http://reviews.llvm.org/D21279#462578, @bmharper wrote:

> 2. friend functions: I don't really understand why the current behavior is 
> what it is, but I think it's reasonable to argue that it actually improves 
> readability by drawing attention to the fact these are friend functions, 
> which ought to be quite rare in most code


Actually it looks like it works now... I'm not sure what I did when I had this 
misalignment. It would have been fine by me anyway.

Regarding constructors, your comment seems reasonable to me. The patch already 
improves the current state, so I think it's good like it is and further 
improvements could be added later on.

Ping @djasper for his review and eventual merge.



Comment at: lib/Format/WhitespaceManager.cpp:95
@@ -97,2 +94,3 @@
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
+  calculateScopeLevel();
   calculateLineBreakInformation();

Maybe we could spare the computation if we aren't going to align anything?

Is it better for clarity to always compute additional information? @djasper 
what's the Clang way to do?



Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

I've taken some time to investigate those two issues, and these are my thoughts:

1. Constructor alignment: I think this is a good thing to do, but if 
`isFunctionDeclarationName`, and it's caller 
`TokenAnnotator::calculateFormattingInformation` are anything to go by, adding 
support for detection of constructors is going to be pretty hairy. I think I 
can see a way to do it, but it involves adding yet more complexity to 
`TokenAnnotator::calculateFormattingInformation`, and I'm not sure it's worth 
the effort. See TokenAnnotator.cpp 

 for reference.

2. friend functions: I don't really understand why the current behavior is what 
it is, but I think it's reasonable to argue that it actually improves 
readability by drawing attention to the fact these are friend functions, which 
ought to be quite rare in most code


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

Thanks! The operators are now correctly handled.

Another thing I've found is that constructors aren't aligned either with other 
declarations or either together. Do you think it would be possible to treat 
them as functions as well?

Friend functions also aren't aligned with the other functions, but I'm not sure 
why or even if they should be. I believe most of the time friend functions are 
declared in a separate declaration "group" anyway.

  struct A {
explicit A(int);
A();
unsigned operator=(int a);
long bar(int a);
friend void foo();
friend unsigned baz();
  };


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper set the repository for this revision to rL LLVM.
bmharper updated this revision to Diff 60830.
bmharper added a comment.

Fix the recent two issues mentioned by Beren, ie the single-statement scopes 
(for loop without braces), and operator[] alignment.


Repository:
  rL LLVM

http://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9242,12 +9242,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  Changes;
   const SourceManager 
   tooling::Replacements Replaces;
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -29,18 +29,18 @@
 bool CreateReplacement, SourceRange OriginalWhitespaceRange,
 unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
 unsigned NewlinesBefore, StringRef PreviousLinePostfix,
-StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective,
-bool IsStartOfDeclName, bool IsInsideToken)
+StringRef CurrentLinePrefix, tok::TokenKind Kind, TokenType Type,
+bool ContinuesPPDirective, bool IsStartOfDeclName, bool IsInsideToken)
 : CreateReplacement(CreateReplacement),
   OriginalWhitespaceRange(OriginalWhitespaceRange),
   StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
   PreviousLinePostfix(PreviousLinePostfix),
-  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind),
+  CurrentLinePrefix(CurrentLinePrefix), Kind(Kind), Type(Type),
   ContinuesPPDirective(ContinuesPPDirective),
   IsStartOfDeclName(IsStartOfDeclName), IndentLevel(IndentLevel),
   Spaces(Spaces), IsInsideToken(IsInsideToken), IsTrailingComment(false),
   TokenLength(0), PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
-  StartOfBlockComment(nullptr), IndentationOffset(0) {}
+  StartOfBlockComment(nullptr), IndentationOffset(0), ScopeLevel(0) {}
 
 void WhitespaceManager::reset() {
   Changes.clear();
@@ -57,9 +57,8 @@
   Changes.push_back(
   Change(/*CreateReplacement=*/true, Tok.WhitespaceRange, IndentLevel,
  Spaces, StartOfTokenColumn, Newlines, "", "", Tok.Tok.getKind(),
- InPPDirective && !Tok.IsFirst,
- Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
- /*IsInsideToken=*/false));
+ Tok.Type, InPPDirective && !Tok.IsFirst,
+ IsStartOfDeclName(Tok), /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::addUntouchableToken(const FormatToken ,
@@ -69,9 +68,8 @@
   Changes.push_back(Change(
   /*CreateReplacement=*/false, Tok.WhitespaceRange, /*IndentLevel=*/0,
   /*Spaces=*/0, Tok.OriginalColumn, Tok.NewlinesBefore, "", "",
-  Tok.Tok.getKind(), InPPDirective && !Tok.IsFirst,
-  Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
-  /*IsInsideToken=*/false));
+  Tok.Tok.getKind(), Tok.Type, InPPDirective && !Tok.IsFirst,
+  IsStartOfDeclName(Tok), /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::replaceWhitespaceInToken(
@@ -85,16 +83,16 @@
   true, SourceRange(Start, Start.getLocWithOffset(ReplaceChars)),
   IndentLevel, Spaces, std::max(0, Spaces), Newlines, PreviousPostfix,
   CurrentPrefix, Tok.is(TT_LineComment) ? tok::comment : tok::unknown,
-  InPPDirective && !Tok.IsFirst,
-  Tok.is(TT_StartOfName) || Tok.is(TT_FunctionDeclarationName),
+  Tok.Type, InPPDirective && !Tok.IsFirst, IsStartOfDeclName(Tok),
   /*IsInsideToken=*/Newlines == 0));
 }
 
 const tooling::Replacements ::generateReplacements() {
   if (Changes.empty())
 return Replaces;
 
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
+  calculateScopeLevel();
   calculateLineBreakInformation();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
@@ -160,59 +158,170 @@
   }
 }
 
+static tok::TokenKind OppositeNestingToken(tok::TokenKind t) {
+  switch (t) {
+  case tok::l_paren:
+return tok::r_paren;
+  case tok::l_brace:
+return tok::r_brace;
+  case tok::l_square:
+return tok::r_square;
+  default:
+assert(false && "Not a nesting token");
+return tok::unknown;
+  }
+}
+
+struct TokenTypeAndLevel {
+  TokenType Type;
+  int ScopeLevel;
+};
+
+// See comment in Change::ScopeLevel for an explanation of why we need
+// to run this function instead of just 

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

Interesting. Working on it!


http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

It looks like it doesn't like the `operator[]` either:

  struct test {
long long int foo();
int operator[](int a);
double bar();
  
long long int foo();
int operator()(int a);
double bar();
  };

becomes:

  struct test {
long long int foo();
int operator[](int a);
double bar();
  
long long int foo();
int   operator()(int a);
doublebar();
  };


http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

There's still cases where the nesting level is still not correctly handled: 
when using unbraced conditionals / loops. For example (sorry, silly example):

  for (auto index = 0, e = 1000; index < e; ++index)
int v = 0;
  long double value = 1;

is aligned to

  for (auto index = 0, e = 1000; index < e; ++index)
int   v = 0;
  long double value = 1;

I'm not sure how to detect these unbraced scopes, maybe by also looking for 
different IndentLevel when computing the ScopeLevel?


http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper removed rL LLVM as the repository for this revision.
bmharper updated this revision to Diff 60569.
bmharper added a comment.

diff with full context


http://reviews.llvm.org/D21279

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9242,12 +9242,11 @@
"};",
Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
   "int i  = 1;\n"
   "SomeType a = SomeFunction(looongParameterA,\n"
   "  loongParameterB);\n"
-  "int j = 2;",
+  "int j  = 2;",
   Alignment);
 
   verifyFormat("template  ScopeStack;
+  for (auto  : Changes) {
+Change.ScopeLevel = ScopeStack.size();
+
+if (ScopeStack.size() != 0 && Change.Kind == ScopeStack.back())
+  ScopeStack.pop_back();
+
+switch (Change.Kind) {
+case tok::l_paren:
+case tok::l_square:
+case tok::l_brace:
+  ScopeStack.push_back(OppositeNestingToken(Change.Kind));
+  break;
+case tok::less:
+  if (Change.Type == TT_TemplateOpener)
+ScopeStack.push_back(tok::greater);
+  break;
+default:
+  break;
+}
+  }
+}
+
 // Align a single sequence of tokens, see AlignTokens below.
 template 
 static void
 AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &,
SmallVector ) {
   bool FoundMatchOnLine = false;
   int Shift = 0;
+
+  // ScopeStack keeps track of the current scope depth (aka NestingLevel).
+  // We only run the "Matches" function on tokens from the outer-most scope.
+  // However, we do need to adjust some special tokens within the entire
+  // Start..End range, regardless of their scope. This special rule applies
+  // only to function parameters which are split across multiple lines, and
+  // who's function names have been shifted for declaration alignment's sake.
+  // See "split function parameter alignment" in FormatTest.cpp for an example.
+  SmallVector ScopeStack;
+
   for (unsigned i = Start; i != End; ++i) {
-if (Changes[i].NewlinesBefore > 0) {
-  FoundMatchOnLine = false;
+if (ScopeStack.size() != 0 &&
+Changes[i].ScopeLevel < ScopeStack.back().ScopeLevel)
+  ScopeStack.pop_back();
+
+if (i != Start) {
+  if (Changes[i].ScopeLevel > Changes[i - 1].ScopeLevel) {
+ScopeStack.push_back({Changes[i - 1].Type, Changes[i].ScopeLevel});
+if (i > Start + 1 && Changes[i - 2].Type == TT_FunctionDeclarationName)
+  ScopeStack.back().Type = Changes[i - 2].Type;
+  }
+}
+
+bool InsideNestedScope = ScopeStack.size() != 0;
+
+if (Changes[i].NewlinesBefore > 0 && !InsideNestedScope) {
   Shift = 0;
+  FoundMatchOnLine = false;
 }
 
 // If this is the first matching token to be aligned, remember by how many
 // spaces it has to be shifted, so the rest of the changes on the line are
 // shifted by the same amount
-if (!FoundMatchOnLine && Matches(Changes[i])) {
+if (!FoundMatchOnLine && !InsideNestedScope && Matches(Changes[i])) {
   FoundMatchOnLine = true;
   Shift = Column - Changes[i].StartOfTokenColumn;
   Changes[i].Spaces += Shift;
 }
 
+// This is for function parameters that are split across multiple lines,
+// as mentioned in the ScopeStack comment.
+if (InsideNestedScope && Changes[i].NewlinesBefore > 0 &&
+ScopeStack.back().Type == TT_FunctionDeclarationName)
+  Changes[i].Spaces += Shift;
+
 assert(Shift >= 0);
 Changes[i].StartOfTokenColumn += Shift;
 if (i + 1 != Changes.size())
   Changes[i + 1].PreviousEndOfTokenColumn += Shift;
   }
 }
 
-// Walk through all of the changes and find sequences of matching tokens to
-// align. To do so, keep track of the lines and whether or not a matching token
-// was found on a line. If a matching token is found, extend the current
-// sequence. If the current line cannot be part of a sequence, e.g. because
-// there is an empty line before it or it contains only non-matching tokens,
-// finalize the previous sequence.
+// Walk through a subset of the changes, starting at StartAt, and find
+// sequences of matching tokens to align. To do so, keep track of the lines and
+// whether or not a matching token was found on a line. If a matching token is
+// found, extend the current sequence. If the current line cannot be part of a
+// sequence, e.g. because there is an empty line before it or it contains only
+// non-matching tokens, finalize the previous sequence.
+// The value returned is the token on which we stopped, either because we
+// exhausted all items inside Changes, or because we hit a scope level higher
+// 

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Beren Minor via cfe-commits]

berenm added a comment.

I think it's better to update the diff with full context (git diff -U99) 
rather than attach the file, in order to have it integrated into phabricator 
diff view. Or use the arcanist command-line tool, which should do it for you.

Except from that, from a quick reading it looks good to me and I'm happy that 
someone fixed theses cases, I see them from time to time actually but never had 
time to work on a proper fix.


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

As requested - full diff (of all three files)

F2059068: AlignFullDiff.patch 


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

Could you upload a diff with full (i.e. the entire file as) context?


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Kevin Funk via cfe-commits]

kfunk added a subscriber: kfunk.
kfunk added a comment.

In http://reviews.llvm.org/D21279#455923, @klimek wrote:

> Generally, please subscribe cfe-commits when sending patches via phab.
>  See http://llvm.org/docs/Phabricator.html


You should set up a Herald rule then: 
https://secure.phabricator.com/book/phabricator/article/herald/ -- you can 
automatically subscribe users based on rules.


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Manuel Klimek via cfe-commits]

klimek added a subscriber: cfe-commits.
klimek added a comment.

Generally, please subscribe cfe-commits when sending patches via phab.
See http://llvm.org/docs/Phabricator.html


Repository:
  rL LLVM

http://reviews.llvm.org/D21279



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