[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D42684



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

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



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

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43015



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

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Also double-checked with Richard Smith and he agrees that the current 
> behavior is preferable. For comma and plus this doesn't seem overly 
> important, but making it:
> 
>   aa(b + ccc *
>  d);
>
> 
> seems really bad to him as this suggests that we are adding both ccc 
> and d.
> 
>   aa(b + ccc *
>  d);
>
> 
> seems clearer.

And I fully agree with this!
But I don't think that would be affected by my patch... I hope it does not at 
least, and I'll try to double check (and add a test).

> And for consistency reasons, we should not treat those two cases differently.

The consistency is related only to the implementation of clang-format: for a 
language (and its users) they are not the same things, comma separating 
arguments are not operators and are used to separate different expressions.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

If people don't care about this case, we might as well merge this :-) Just 
kidding.

The tweak matches both our expectation, the auto-indent behaviour of IDE (not 
to be trusted, but still probably of 'default' behaviour for many people, esp. 
when you don't yet use a formatter), and its seems our code base is generally 
formatted like this. I think it is quite more frequent than 50 times in whole 
LLVM/Clang, but I have no actual numbers; do you have a magic tool to search 
for such specific "code pattern", or just run clang-format with one option then 
the next and count the differences ?


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136557.
Typz added a comment.

Change options values to No/MultiLine/Yes.


Repository:
  rC Clang

https://reviews.llvm.org/D42684

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5445,7 +5445,7 @@
"const typename  aaa);");
 
   FormatStyle AlwaysBreak = getLLVMStyle();
-  AlwaysBreak.AlwaysBreakTemplateDeclarations = true;
+  AlwaysBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
   verifyFormat("template \nclass C {};", AlwaysBreak);
   verifyFormat("template \nvoid f();", AlwaysBreak);
   verifyFormat("template \nvoid f() {}", AlwaysBreak);
@@ -5463,6 +5463,32 @@
"public:\n"
"  E *f();\n"
"};");
+
+  FormatStyle NeverBreak = getLLVMStyle();
+  NeverBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_No;
+  verifyFormat("template  class C {};", NeverBreak);
+  verifyFormat("template  void f();", NeverBreak);
+  verifyFormat("template  void f() {}", NeverBreak);
+  verifyFormat("template \nvoid foo(aa ) {}",
+   NeverBreak);
+  verifyFormat("void aaa(\n"
+   "ccc);",
+   NeverBreak);
+  verifyFormat("template  class Fooo,\n"
+   "  template  class Baaar>\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  // T can be A, B or C.\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  class A {\n"
+   "public:\n"
+   "  E *f();\n"
+   "};", NeverBreak);
+  NeverBreak.PenaltyBreakTemplateDeclaration = 100;
+  verifyFormat("template  void\nfoo(aa ) {}",
+   NeverBreak);
 }
 
 TEST_F(FormatTest, WrapsTemplateParameters) {
@@ -10352,7 +10378,6 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
-  CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
@@ -10418,6 +10443,8 @@
   PenaltyBreakAssignment, 1234u);
   CHECK_PARSE("PenaltyBreakBeforeFirstCallParameter: 1234",
   PenaltyBreakBeforeFirstCallParameter, 1234u);
+  CHECK_PARSE("PenaltyBreakTemplateDeclaration: 1234",
+  PenaltyBreakTemplateDeclaration, 1234u);
   CHECK_PARSE("PenaltyExcessCharacter: 1234", PenaltyExcessCharacter, 1234u);
   CHECK_PARSE("PenaltyReturnTypeOnItsOwnLine: 1234",
   PenaltyReturnTypeOnItsOwnLine, 1234u);
@@ -10572,6 +10599,18 @@
   AlwaysBreakAfterReturnType,
   FormatStyle::RTBS_TopLevelDefinitions);
 
+  Style.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: No", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_No);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: MultiLine", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_MultiLine);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: Yes", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_Yes);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: false", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_MultiLine);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: true", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_Yes);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
   AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2244,6 +2244,8 @@
   return 2;
 return 1;
   }
+  if (Left.ClosesTemplateDeclaration)
+  return Style.PenaltyBreakTemplateDeclaration;
   if (Left.is(TT_ConditionalExpr))
 return prec::Conditional;
   prec::Level Level = Left.getPrecedence();
@@ -2753,7 +2755,7 @@
   if (Right.Previous->ClosesTemplateDeclaration &&
   Right.Previous->MatchingParen &&
   Right.Previous->MatchingParen->NestingLevel == 0 &&
-  Style.AlwaysBreakTemplateDeclarations)
+  Style.AlwaysBreakTemplateDeclarations == FormatStyle::BTDS_Yes)
  

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43015



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

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC326426: [clang-format] Add SpaceBeforeColon option (authored 
by Typz, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D32525?vs=136305=136483#toc

Repository:
  rC Clang

https://reviews.llvm.org/D32525

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: docs/ClangFormatStyleOptions.rst
===
--- docs/ClangFormatStyleOptions.rst
+++ docs/ClangFormatStyleOptions.rst
@@ -1681,6 +1681,23 @@
  int a = 5; vs. int a=5;
  a += 42a+=42;
 
+**SpaceBeforeCtorInitializerColon** (``bool``)
+  If ``false``, spaces will be removed before constructor initializer
+  colon.
+
+  .. code-block:: c++
+
+ true:  false:
+ Foo::Foo() : a(a) {}   Foo::Foo(): a(a) {}
+
+**SpaceBeforeInheritanceColon** (``bool``)
+  If ``false``, spaces will be removed before inheritance colon.
+
+  .. code-block:: c++
+
+ true:  false:
+ class Foo : Bar {} vs. class Foo: Bar {}
+
 **SpaceBeforeParens** (``SpaceBeforeParensOptions``)
   Defines in which cases to put a space before opening parentheses.
 
@@ -1725,6 +1742,15 @@
 
 
 
+**SpaceBeforeRangeBasedForLoopColon** (``bool``)
+  If ``false``, spaces will be removed before range-based for loop
+  colon.
+
+  .. code-block:: c++
+
+ true:  false:
+ for (auto v : values) {}   vs. for(auto v: values) {}
+
 **SpaceInEmptyParentheses** (``bool``)
   If ``true``, spaces may be inserted into ``()``.
 
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -1525,6 +1525,21 @@
   /// \endcode
   bool SpaceBeforeAssignmentOperators;
 
+  /// \brief If ``false``, spaces will be removed before constructor initializer
+  /// colon.
+  /// \code
+  ///true:  false:
+  ///Foo::Foo() : a(a) {}   Foo::Foo(): a(a) {}
+  /// \endcode
+  bool SpaceBeforeCtorInitializerColon;
+
+  /// \brief If ``false``, spaces will be removed before inheritance colon.
+  /// \code
+  ///true:  false:
+  ///class Foo : Bar {} vs. class Foo: Bar {}
+  /// \endcode
+  bool SpaceBeforeInheritanceColon;
+
   /// \brief Different ways to put a space before opening parentheses.
   enum SpaceBeforeParensOptions {
 /// Never put a space before opening parentheses.
@@ -1563,6 +1578,14 @@
   /// \brief Defines in which cases to put a space before opening parentheses.
   SpaceBeforeParensOptions SpaceBeforeParens;
 
+  /// \brief If ``false``, spaces will be removed before range-based for loop
+  /// colon.
+  /// \code
+  ///true:  false:
+  ///for (auto v : values) {}   vs. for(auto v: values) {}
+  /// \endcode
+  bool SpaceBeforeRangeBasedForLoopColon;
+
   /// \brief If ``true``, spaces may be inserted into ``()``.
   /// \code
   ///true:false:
@@ -1744,7 +1767,12 @@
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
+   SpaceBeforeCtorInitializerColon ==
+   R.SpaceBeforeCtorInitializerColon &&
+   SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
SpaceBeforeParens == R.SpaceBeforeParens &&
+   SpaceBeforeRangeBasedForLoopColon ==
+   R.SpaceBeforeRangeBasedForLoopColon &&
SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
SpacesInAngles == R.SpacesInAngles &&
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2575,8 +2575,15 @@
 return true;
   if (Right.is(tok::comma))
 return false;
-  if (Right.isOneOf(TT_CtorInitializerColon, TT_ObjCBlockLParen))
+  if (Right.is(TT_ObjCBlockLParen))
 return true;
+  if (Right.is(TT_CtorInitializerColon))
+return Style.SpaceBeforeCtorInitializerColon;
+  if (Right.is(TT_InheritanceColon) && !Style.SpaceBeforeInheritanceColon)
+return false;
+  if (Right.is(TT_RangeBasedForLoopColon) &&
+  !Style.SpaceBeforeRangeBasedForLoopColon)
+return false;
   if (Right.is(tok::colon)) {
 if (Line.First->isOneOf(tok::kw_case, tok::kw_default) ||
 !Right.getNextNonComment() || 

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326426: [clang-format] Add SpaceBeforeColon option (authored 
by Typz, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D32525

Files:
  cfe/trunk/docs/ClangFormatStyleOptions.rst
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/docs/ClangFormatStyleOptions.rst
===
--- cfe/trunk/docs/ClangFormatStyleOptions.rst
+++ cfe/trunk/docs/ClangFormatStyleOptions.rst
@@ -1681,6 +1681,23 @@
  int a = 5; vs. int a=5;
  a += 42a+=42;
 
+**SpaceBeforeCtorInitializerColon** (``bool``)
+  If ``false``, spaces will be removed before constructor initializer
+  colon.
+
+  .. code-block:: c++
+
+ true:  false:
+ Foo::Foo() : a(a) {}   Foo::Foo(): a(a) {}
+
+**SpaceBeforeInheritanceColon** (``bool``)
+  If ``false``, spaces will be removed before inheritance colon.
+
+  .. code-block:: c++
+
+ true:  false:
+ class Foo : Bar {} vs. class Foo: Bar {}
+
 **SpaceBeforeParens** (``SpaceBeforeParensOptions``)
   Defines in which cases to put a space before opening parentheses.
 
@@ -1725,6 +1742,15 @@
 
 
 
+**SpaceBeforeRangeBasedForLoopColon** (``bool``)
+  If ``false``, spaces will be removed before range-based for loop
+  colon.
+
+  .. code-block:: c++
+
+ true:  false:
+ for (auto v : values) {}   vs. for(auto v: values) {}
+
 **SpaceInEmptyParentheses** (``bool``)
   If ``true``, spaces may be inserted into ``()``.
 
Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1525,6 +1525,21 @@
   /// \endcode
   bool SpaceBeforeAssignmentOperators;
 
+  /// \brief If ``false``, spaces will be removed before constructor initializer
+  /// colon.
+  /// \code
+  ///true:  false:
+  ///Foo::Foo() : a(a) {}   Foo::Foo(): a(a) {}
+  /// \endcode
+  bool SpaceBeforeCtorInitializerColon;
+
+  /// \brief If ``false``, spaces will be removed before inheritance colon.
+  /// \code
+  ///true:  false:
+  ///class Foo : Bar {} vs. class Foo: Bar {}
+  /// \endcode
+  bool SpaceBeforeInheritanceColon;
+
   /// \brief Different ways to put a space before opening parentheses.
   enum SpaceBeforeParensOptions {
 /// Never put a space before opening parentheses.
@@ -1563,6 +1578,14 @@
   /// \brief Defines in which cases to put a space before opening parentheses.
   SpaceBeforeParensOptions SpaceBeforeParens;
 
+  /// \brief If ``false``, spaces will be removed before range-based for loop
+  /// colon.
+  /// \code
+  ///true:  false:
+  ///for (auto v : values) {}   vs. for(auto v: values) {}
+  /// \endcode
+  bool SpaceBeforeRangeBasedForLoopColon;
+
   /// \brief If ``true``, spaces may be inserted into ``()``.
   /// \code
   ///true:false:
@@ -1744,7 +1767,12 @@
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
+   SpaceBeforeCtorInitializerColon ==
+   R.SpaceBeforeCtorInitializerColon &&
+   SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
SpaceBeforeParens == R.SpaceBeforeParens &&
+   SpaceBeforeRangeBasedForLoopColon ==
+   R.SpaceBeforeRangeBasedForLoopColon &&
SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
SpacesInAngles == R.SpacesInAngles &&
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2575,8 +2575,15 @@
 return true;
   if (Right.is(tok::comma))
 return false;
-  if (Right.isOneOf(TT_CtorInitializerColon, TT_ObjCBlockLParen))
+  if (Right.is(TT_ObjCBlockLParen))
 return true;
+  if (Right.is(TT_CtorInitializerColon))
+return Style.SpaceBeforeCtorInitializerColon;
+  if (Right.is(TT_InheritanceColon) && !Style.SpaceBeforeInheritanceColon)
+return false;
+  if (Right.is(TT_RangeBasedForLoopColon) &&
+  !Style.SpaceBeforeRangeBasedForLoopColon)
+return false;
   if (Right.is(tok::colon)) {
 

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D42787#1022081, @djasper wrote:

> But you *do* want extra indentation in the case of:
>
>   function(a, 
>b +
>cc);
>   
>
> I understand you argument, but I don't agree at the moment. As is (without 
> getting more feedback from others that clang-format is behaving unexpected 
> here), I do not want to move forward with this change.


Indeed, because as much as we want to keep things aligned (and avoid extra 
indentation), we want above all to make code "easier to read": we think keeping 
alignment is indeed helping, but that keeping the alignment when it adds 
confusion is not...
Now let's hear what others are thinking about this behavior.

(BTW, I imagine there is no point introducing an option for this specific, if 
this does not get accepted as default? It may be done as part of 
https://reviews.llvm.org/D32478 to minimize options, in the new mode which 
tries to align more strictly... But last time you looked at it you were not 
very enthousiastic about it either...)


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Indeed, seems to apply to classes as well. Maybe I was mislead by my testing, 
where I did not get the case (possibly because we use 
`ConstructorInitializerAllOnOneLineOrOnePerLine=true`, so the continuation 
indenter only sees "short" class declarations unless breaking the template is 
required because the name is too long).

So just to be clear, are you saying the whole approach is not the right one, or 
simply that the "names" of each modes are not?
For the name, maybe something like may be better:

- `Never`
- `MultiLineDeclaration`, or maybe even `MultiLine`
- `Always`


Repository:
  rC Clang

https://reviews.llvm.org/D42684



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

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136306.
Typz added a comment.

Address review comments of https://reviews.llvm.org/D32525


Repository:
  rC Clang

https://reviews.llvm.org/D43015

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1286,15 +1286,40 @@
   verifyFormat("class ::A::B {};");
 }
 
-TEST_F(FormatTest, BreakBeforeInheritanceComma) {
-  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
-  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
-
-  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+TEST_F(FormatTest, BreakInheritanceStyle) {
+  FormatStyle StyleWithInheritanceBreakBeforeComma = getLLVMStyle();
+  StyleWithInheritanceBreakBeforeComma.BreakInheritanceList =
+  FormatStyle::BILS_BeforeComma;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakBeforeComma);
   verifyFormat("class MyClass\n"
": public X\n"
", public Y {};",
-   StyleWithInheritanceBreak);
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("class AA\n"
+   ": public BB\n"
+   ", public CC {};",
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("struct a\n"
+   ": public aaa< // break\n"
+   "  > {};",
+   StyleWithInheritanceBreakBeforeComma);
+
+  FormatStyle StyleWithInheritanceBreakAfterColon = getLLVMStyle();
+  StyleWithInheritanceBreakAfterColon.BreakInheritanceList =
+  FormatStyle::BILS_AfterColon;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class MyClass : public X, public Y {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class AA :\n"
+   "public BB,\n"
+   "public CC {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("struct a :\n"
+   "public aaa< // break\n"
+   "> {};",
+   StyleWithInheritanceBreakAfterColon);
 }
 
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
@@ -3696,6 +3721,23 @@
"  aa,\n"
"  bb {}",
Style);
+
+  // `ConstructorInitializerIndentWidth` actually applies to InheritanceList as well
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
+  verifyFormat("class SomeClass\n"
+   "  : public aa,\n"
+   "public bb {};",
+   Style);
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeComma;
+  verifyFormat("class SomeClass\n"
+   "  : public aa\n"
+   "  , public bb {};",
+   Style);
+  Style.BreakInheritanceList = FormatStyle::BILS_AfterColon;
+  verifyFormat("class SomeClass :\n"
+   "  public aa,\n"
+   "  public bb {};",
+   Style);
 }
 
 #ifndef EXPENSIVE_CHECKS
@@ -8987,7 +9029,7 @@
"  (2) {}",
CtorInitializerStyle);
 
-  FormatStyle InheritanceStyle = getLLVMStyle();
+  FormatStyle InheritanceStyle = getLLVMStyleWithColumns(30);
   InheritanceStyle.SpaceBeforeInheritanceColon = false;
   verifyFormat("class Foo: public Bar {};", InheritanceStyle);
   verifyFormat("Foo::Foo() : foo(1) {}", InheritanceStyle);
@@ -9003,6 +9045,29 @@
"default:\n"
"}",
InheritanceStyle);
+  InheritanceStyle.BreakInheritanceList = FormatStyle::BILS_AfterColon;
+  verifyFormat("class Foo:\n"
+   "public aa,\n"
+   "public bb {\n"
+   "}",
+   InheritanceStyle);
+  InheritanceStyle.BreakInheritanceList = FormatStyle::BILS_BeforeComma;
+  verifyFormat("class Foo\n"
+   ": public aa\n"
+   ", public bb {\n"
+  

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: unittests/Format/FormatTest.cpp:8969
+   "barr(1) {}", CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers = 
FormatStyle::BCIS_BeforeComma;
+  verifyFormat("Fooo::Fooo()\n"

djasper wrote:
> This is a useless test if it doesn't actually have multiple initializers 
> separated by a comma.
since we are interested only in the behavior next to the colon, I think it is 
relevant... But I'll add extra parameter to make it more realistic,


Repository:
  rC Clang

https://reviews.llvm.org/D32525



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

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136305.
Typz marked 2 inline comments as done.
Typz added a comment.

Address review comments


Repository:
  rC Clang

https://reviews.llvm.org/D32525

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8933,6 +8933,114 @@
   verifyFormat("a or_eq 8;", Spaces);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeColon) {
+  verifyFormat("class Foo : public Bar {};");
+  verifyFormat("Foo::Foo() : foo(1) {}");
+  verifyFormat("for (auto a : b) {\n}");
+  verifyFormat("int x = a ? b : c;");
+  verifyFormat("{\n"
+   "label0:\n"
+   "  int x = 0;\n"
+   "}");
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}");
+
+  FormatStyle CtorInitializerStyle = getLLVMStyleWithColumns(30);
+  CtorInitializerStyle.SpaceBeforeCtorInitializerColon = false;
+  verifyFormat("class Foo : public Bar {};", CtorInitializerStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", CtorInitializerStyle);
+  verifyFormat("for (auto a : b) {\n}", CtorInitializerStyle);
+  verifyFormat("int x = a ? b : c;", CtorInitializerStyle);
+  verifyFormat("{\n"
+   "label1:\n"
+   "  int x = 0;\n"
+   "}",
+   CtorInitializerStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers =
+  FormatStyle::BCIS_AfterColon;
+  verifyFormat("Fooo::Fooo():\n"
+   "(1),\n"
+   "(2) {}",
+   CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers =
+  FormatStyle::BCIS_BeforeComma;
+  verifyFormat("Fooo::Fooo()\n"
+   ": (1)\n"
+   ", (2) {}",
+   CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers =
+  FormatStyle::BCIS_BeforeColon;
+  verifyFormat("Fooo::Fooo()\n"
+   ": (1),\n"
+   "  (2) {}",
+   CtorInitializerStyle);
+  CtorInitializerStyle.ConstructorInitializerIndentWidth = 0;
+  verifyFormat("Fooo::Fooo()\n"
+   ": (1),\n"
+   "  (2) {}",
+   CtorInitializerStyle);
+
+  FormatStyle InheritanceStyle = getLLVMStyle();
+  InheritanceStyle.SpaceBeforeInheritanceColon = false;
+  verifyFormat("class Foo: public Bar {};", InheritanceStyle);
+  verifyFormat("Foo::Foo() : foo(1) {}", InheritanceStyle);
+  verifyFormat("for (auto a : b) {\n}", InheritanceStyle);
+  verifyFormat("int x = a ? b : c;", InheritanceStyle);
+  verifyFormat("{\n"
+   "label2:\n"
+   "  int x = 0;\n"
+   "}",
+   InheritanceStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   InheritanceStyle);
+
+  FormatStyle ForLoopStyle = getLLVMStyle();
+  ForLoopStyle.SpaceBeforeRangeBasedForLoopColon = false;
+  verifyFormat("class Foo : public Bar {};", ForLoopStyle);
+  verifyFormat("Foo::Foo() : foo(1) {}", ForLoopStyle);
+  verifyFormat("for (auto a: b) {\n}", ForLoopStyle);
+  verifyFormat("int x = a ? b : c;", ForLoopStyle);
+  verifyFormat("{\n"
+   "label2:\n"
+   "  int x = 0;\n"
+   "}",
+   ForLoopStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   ForLoopStyle);
+
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  NoSpaceStyle.SpaceBeforeCtorInitializerColon = false;
+  NoSpaceStyle.SpaceBeforeInheritanceColon = false;
+  NoSpaceStyle.SpaceBeforeRangeBasedForLoopColon = false;
+  verifyFormat("class Foo: public Bar {};", NoSpaceStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", NoSpaceStyle);
+  verifyFormat("for (auto a: b) {\n}", NoSpaceStyle);
+  verifyFormat("int x = a ? b : c;", NoSpaceStyle);
+  verifyFormat("{\n"
+   "label3:\n"
+   "  int x = 0;\n"
+   "}",
+   NoSpaceStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   NoSpaceStyle);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignments) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveAssignments = false;
@@ -10274,6 +10382,9 @@
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136300.
Typz marked an inline comment as done.
Typz added a comment.

Address review comments, and rebase on https://reviews.llvm.org/D32525


Repository:
  rC Clang

https://reviews.llvm.org/D43015

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1286,15 +1286,40 @@
   verifyFormat("class ::A::B {};");
 }
 
-TEST_F(FormatTest, BreakBeforeInheritanceComma) {
-  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
-  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
-
-  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+TEST_F(FormatTest, BreakInheritanceStyle) {
+  FormatStyle StyleWithInheritanceBreakBeforeComma = getLLVMStyle();
+  StyleWithInheritanceBreakBeforeComma.BreakInheritanceList =
+  FormatStyle::BILS_BeforeComma;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakBeforeComma);
   verifyFormat("class MyClass\n"
": public X\n"
", public Y {};",
-   StyleWithInheritanceBreak);
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("class AA\n"
+   ": public BB\n"
+   ", public CC {};",
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("struct a\n"
+   ": public aaa< // break\n"
+   "  > {};",
+   StyleWithInheritanceBreakBeforeComma);
+
+  FormatStyle StyleWithInheritanceBreakAfterColon = getLLVMStyle();
+  StyleWithInheritanceBreakAfterColon.BreakInheritanceList =
+  FormatStyle::BILS_AfterColon;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class MyClass : public X, public Y {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class AA :\n"
+   "public BB,\n"
+   "public CC {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("struct a :\n"
+   "public aaa< // break\n"
+   "> {};",
+   StyleWithInheritanceBreakAfterColon);
 }
 
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
@@ -3696,6 +3721,23 @@
"  aa,\n"
"  bb {}",
Style);
+
+  // `ConstructorInitializerIndentWidth` actually applies to InheritanceList as well
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
+  verifyFormat("class SomeClass\n"
+   "  : public aa,\n"
+   "public bb {};",
+   Style);
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeComma;
+  verifyFormat("class SomeClass\n"
+   "  : public aa\n"
+   "  , public bb {};",
+   Style);
+  Style.BreakInheritanceList = FormatStyle::BILS_AfterColon;
+  verifyFormat("class SomeClass :\n"
+   "  public aa,\n"
+   "  public bb {};",
+   Style);
 }
 
 #ifndef EXPENSIVE_CHECKS
@@ -8976,7 +9018,7 @@
   verifyFormat("Fooo::Fooo()\n"
": barr(1) {}", CtorInitializerStyle);
 
-  FormatStyle InheritanceStyle = getLLVMStyle();
+  FormatStyle InheritanceStyle = getLLVMStyleWithColumns(30);
   InheritanceStyle.SpaceBeforeInheritanceColon = false;
   verifyFormat("class Foo: public Bar {};", InheritanceStyle);
   verifyFormat("Foo::Foo() : foo(1) {}", InheritanceStyle);
@@ -8992,6 +9034,21 @@
"default:\n"
"}",
InheritanceStyle);
+  InheritanceStyle.BreakInheritanceList = FormatStyle::BILS_AfterColon;
+  verifyFormat("class Foo:\n"
+   "public Ba {\n"
+   "}", InheritanceStyle);
+  InheritanceStyle.BreakInheritanceList = FormatStyle::BILS_BeforeComma;
+  verifyFormat("class Foo\n"
+   ": public Ba {\n"
+   "}", InheritanceStyle);
+ 

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136299.
Typz marked 2 inline comments as done.
Typz added a comment.

Address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D32525

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8933,6 +8933,103 @@
   verifyFormat("a or_eq 8;", Spaces);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeColon) {
+  verifyFormat("class Foo : public Bar {};");
+  verifyFormat("Foo::Foo() : foo(1) {}");
+  verifyFormat("for (auto a : b) {\n}");
+  verifyFormat("int x = a ? b : c;");
+  verifyFormat("{\n"
+   "label0:\n"
+   "  int x = 0;\n"
+   "}");
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}");
+
+  FormatStyle CtorInitializerStyle = getLLVMStyleWithColumns(30);
+  CtorInitializerStyle.SpaceBeforeCtorInitializerColon = false;
+  verifyFormat("class Foo : public Bar {};", CtorInitializerStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", CtorInitializerStyle);
+  verifyFormat("for (auto a : b) {\n}", CtorInitializerStyle);
+  verifyFormat("int x = a ? b : c;", CtorInitializerStyle);
+  verifyFormat("{\n"
+   "label1:\n"
+   "  int x = 0;\n"
+   "}",
+   CtorInitializerStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+  verifyFormat("Fooo::Fooo():\n"
+   "barr(1) {}", CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  verifyFormat("Fooo::Fooo()\n"
+   ": barr(1) {}", CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+  verifyFormat("Fooo::Fooo()\n"
+   ": barr(1) {}", CtorInitializerStyle);
+  CtorInitializerStyle.ConstructorInitializerIndentWidth = 0;
+  verifyFormat("Fooo::Fooo()\n"
+   ": barr(1) {}", CtorInitializerStyle);
+
+  FormatStyle InheritanceStyle = getLLVMStyle();
+  InheritanceStyle.SpaceBeforeInheritanceColon = false;
+  verifyFormat("class Foo: public Bar {};", InheritanceStyle);
+  verifyFormat("Foo::Foo() : foo(1) {}", InheritanceStyle);
+  verifyFormat("for (auto a : b) {\n}", InheritanceStyle);
+  verifyFormat("int x = a ? b : c;", InheritanceStyle);
+  verifyFormat("{\n"
+   "label2:\n"
+   "  int x = 0;\n"
+   "}",
+   InheritanceStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   InheritanceStyle);
+
+  FormatStyle ForLoopStyle = getLLVMStyle();
+  ForLoopStyle.SpaceBeforeRangeBasedForLoopColon = false;
+  verifyFormat("class Foo : public Bar {};", ForLoopStyle);
+  verifyFormat("Foo::Foo() : foo(1) {}", ForLoopStyle);
+  verifyFormat("for (auto a: b) {\n}", ForLoopStyle);
+  verifyFormat("int x = a ? b : c;", ForLoopStyle);
+  verifyFormat("{\n"
+   "label2:\n"
+   "  int x = 0;\n"
+   "}",
+   ForLoopStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   ForLoopStyle);
+
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  NoSpaceStyle.SpaceBeforeCtorInitializerColon = false;
+  NoSpaceStyle.SpaceBeforeInheritanceColon = false;
+  NoSpaceStyle.SpaceBeforeRangeBasedForLoopColon = false;
+  verifyFormat("class Foo: public Bar {};", NoSpaceStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", NoSpaceStyle);
+  verifyFormat("for (auto a: b) {\n}", NoSpaceStyle);
+  verifyFormat("int x = a ? b : c;", NoSpaceStyle);
+  verifyFormat("{\n"
+   "label3:\n"
+   "  int x = 0;\n"
+   "}",
+   NoSpaceStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   NoSpaceStyle);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignments) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveAssignments = false;
@@ -10274,6 +10371,9 @@
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
+  CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
+  CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
+  CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
 
   

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

The problem I have is really related to the current 
`AlwaysBreakTemplateDeclarations` behavior, which does not apply to functions.
I set it to false, and I get this:

  template<>
  void (
  const bbb & cc);

instead of:

  template<> void (
  const bbb & cc);

Then when this is fixed the penalty for breaking after the templates part is 
hardcoded to 10 (`prec::Level::Relational`), which was not always wrapping as 
expected (that is definitely subjective, but that is the beauty of penalties...)


Repository:
  rC Clang

https://reviews.llvm.org/D42684



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

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D32525



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Some initial design work has been done, and Krasimir said that he's 
> interested. No timeline though :(

any update or progress maybe?


https://reviews.llvm.org/D37813



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

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D42684#1013005, @djasper wrote:

> Please given an explanation of what you are trying to achieve with this 
> change. Do you intend to set the penalty high so that clang-format does other 
> things first before falling back to wrapping template declarations?
>
> Why treat separate declarations differently wrt. wrapping the template 
> declarations? Will this stop at classes vs. functions? I generally have 
> doubts that this option carries it's weight. Do you have a style guide that 
> explicitly tells you to do this?


I set the penalty relatively high (200), i.e. the same penalty that I have set 
for `PenaltyBreakAssignment`, `PenaltyBreakBeforeFirstCallParameter`, and 
`PenaltyReturnTypeOnItsOwnLine`.
Basically we don't have a preference as to where the break should be, and we 
found that setting the same penalty for all these parameters (including the new 
`PenaltyBreakTemplateDeclaration`) gives really "natural" result: e.g. very 
close to how a user would optimize the code wrapping for readability.

At the moment `AlwaysBreakTemplateDeclarations` actually applies only to 
classes, thus templates are always wrapped before functions (unless it all fits 
on one line). A simpler change would have been to make 
`AlwaysBreakTemplateDeclarations` stick to its name, and let it apply to 
functions as well: but I figured with would break existing clang-format styles.


Repository:
  rC Clang

https://reviews.llvm.org/D42684



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

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D42729#994841, @djasper wrote:

> - Of course you find all sorts of errors while testing clang-format on a 
> large-enough codebase. That doesn't mean that users run into them much.
> - We have had about 10k clang-format users internally for several years. The 
> semicolon issue comes up but really rarely and if it does, people happily fix 
> their code not blaming clang-format.
>
>   Unrelated, my point remains that setting BlockKind in TokenAnnotator is bad 
> enough that I wouldn't want to do it for reaping this small benefit. And I 
> can't see how you could easily achieve the same thing without doing that.


Just a question though. I there a reason brace matching (and other parts of 
TokenAnnotations) are not performed before LineUnwrapping? That would probably 
allow fixing this issue more cleanly (though I am not sure I would have the 
time to actually perform this probably significant task)...


Repository:
  rC Clang

https://reviews.llvm.org/D42729



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D42787#994781, @djasper wrote:

> What I mean is, users will find it surprising if whether or not a parameter 
> gets wrapped leads to a different indentation internal to that parameter. I 
> have not heard of a single user that would be surprised by this extra 
> indentation.


well you have now...
I configured the tool to align AlignOperands, to I expected the operands of my 
expressions to be aligned, even if they are in a function call.

(Btw, I don't think the comma used to separate function parameters are actually 
considered "operators" by the C++ standard, so technically this is not a case 
of multiple precedences, and users may not expect it to be handled this way...)


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> I don't agree that that's the same thing. The closing brace is still neatly 
> aligned with the line of the opening brace (which happens to be just the 
> opening brace).

This invariant is not really applicable to switch statements, where code of 
each "branch" is already indented with no opening brace. :-)

I can try to change the mode so that we get either "google" style:

  switch (x) {
  case 0: {
foo():
  } break;
  }

or "regular indentation" mode (e.g. what would look like no special indentation 
rule):

  switch (x) {
  case 0:
{
  foo():
}
break;
  }

Generally, would a flag for `CaseBlockIndent` be acceptable for this purpose ?
Or should the behavior simply be selected depending on `IndentCaseLabels` ?


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43015



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

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2183
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)

djasper wrote:
> Why is this necessary?
Otherwise the `PenaltyBreakBeforeFirstCallParameter` is used, which is not 
consistent with the concept of !Cpp11BraceListStyle (e.g. consider this is an 
initializer), and gives the following format, which is certainly not the 
expected result:

  const std::unordered_map
  MyHashTable = { { \"a\", 0 },
  { \"b\", 1 },
  { \"c\", 2 } };

(again, the issue only happens when `PenaltyBreakBeforeFirstCallParameter` is 
increased, e.g. 200 in my case. The default value is 19, so formatting is not 
affected)



Comment at: unittests/Format/FormatTest.cpp:6655
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"

djasper wrote:
> How does this penalty influence the test?
Because breaking after the opening brace is affected by the 
`PenaltyBreakBeforeFirstCallParameter` : this is an init braced-list, but it is 
considered as any function.

Specifically, my patch needs (see prev. comment) to change the penalty for 
breaking after the brace, but this applies only when 
`Cpp11BracedListStyle=false`, as per your earlier comment. So this test just 
verify that the behavior is indeed not affected.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



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

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136081.
Typz added a comment.

Prevent breaking between = and { when Cpp11BracedListStyle=false.


Repository:
  rC Clang

https://reviews.llvm.org/D43290

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,9 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }
@@ -2863,6 +2866,9 @@
   if (Left.is(tok::equal) && !Right.isOneOf(tok::kw_default, tok::kw_delete) &&
   Line.Type == LT_VirtualFunctionDecl && Left.NestingLevel == 0)
 return false;
+  if (Left.is(tok::equal) && Right.is(tok::l_brace) &&
+  !Style.Cpp11BracedListStyle)
+return false;
   if (Left.is(tok::l_paren) && Left.is(TT_AttributeParen))
 return false;
   if (Left.is(tok::l_paren) && Left.Previous &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,9 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)
+  return 19;
 return 

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

>> Tweaking the penalty handling would still be needed in any case, since the 
>> problem happens also when Cpp11BracedListStyle is true (though the effect is 
>> more subtle)
> 
> I don't understand this. Which style do you actually care about? With 
> Cpp11BracedListStyle=true or =false?

I use the `Cpp11BracedListStyle=false` style.
However, I think the current behavior is wrong also when 
`Cpp11BracedListStyle=true`. Here the behavior in this case:

Before :

  const std::unordered_map Something::MyHashTable =
  {{ "a", 0 },
   { "b", 1 },
   { "c", 2 }};

After :

  const std::unordered_set Something::MyUnorderedSet = {
  { "a", 0 },
  { "b", 1 },
  { "c", 2 }};

>> Generally, I think it is better to just rely on penalties, since it gives a 
>> way to compare and weight each solution. Then each style can decide what 
>> should break first: e.g. a style may also have a lower 
>> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be 
>> expected...
> 
> And with my reasoning, I think exactly the opposite. Penalties are nice, but 
> if the behavior is generally unwanted, than it's very hard to predict in 
> which situations it might still occur.

Yet on the other hand enforcing too much can lead to cases where the optimizer 
fails to find a solution, and ends up simply not formatting the line: which is 
fine is the code is already formatted (but if there were the case we would not 
need the tool at all :-) )
The beauty of penalties is that one can tweak the different penalties so that 
the behavior is "generally" what would be expected: definitely not predictable, 
but then there are always cases where the "regular" style does not work, and 
fallback solutions must be used... (for exemple, I would prefer never to never 
break after an assignment : yet sometimes it is needed...)

I can change the patch to disallow breaking, but this would be a slightly more 
risky change, with possibly more side-effects; and i think that would not solve 
the issue when `Cpp11BracedListStyle=true`.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326191: clang-format: fix formatting of ObjC @synchronized 
blocks (authored by Typz, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43114?vs=133570=136067#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43114

Files:
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp


Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1135,6 +1135,18 @@
 }
 addUnwrappedLine();
 return;
+  case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+   // Skip synchronization object
+   parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+  parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
   case tok::objc_try:
 // This branch isn't strictly necessary (the kw_try case below would
 // do this too after the tok::at is parsed above).  But be explicit.
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -209,6 +209,24 @@
"a);\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+  verifyFormat("@synchronized(self) {\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self) {\n"
+   "  f();\n"
+   "}\n");
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  verifyFormat("@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"


Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1135,6 +1135,18 @@
 }
 addUnwrappedLine();
 return;
+  case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+   // Skip synchronization object
+   parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+  parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
   case tok::objc_try:
 // This branch isn't strictly necessary (the kw_try case below would
 // do this too after the tok::at is parsed above).  But be explicit.
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -209,6 +209,24 @@
"a);\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+  verifyFormat("@synchronized(self) {\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self) {\n"
+   "  f();\n"
+   "}\n");
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  verifyFormat("@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326192: clang-format: use AfterControlStatement to format 
ObjC control blocks (authored by Typz, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D43232

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp


Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1129,7 +1129,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1141,7 +1141,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
@@ -314,6 +314,14 @@
   }
   return MergedLines;
 }
+// Don't merge block with left brace wrapped after ObjC special blocks
+if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
+I[-1]->First->is(tok::at) && I[-1]->First->Next) {
+  tok::ObjCKeywordKind kwId = I[-1]->First->Next->Tok.getObjCKeywordID();
+  if (kwId == clang::tok::objc_autoreleasepool ||
+  kwId == clang::tok::objc_synchronized)
+return 0;
+}
 // Try to merge a block with left brace wrapped that wasn't yet covered
 if (TheLine->Last->is(tok::l_brace)) {
   return !Style.BraceWrapping.AfterFunction ||
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -187,7 +187,8 @@
"@autoreleasepool {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@autoreleasepool\n"
"{\n"
"  f();\n"
@@ -216,7 +217,8 @@
"@synchronized(self) {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@synchronized(self)\n"
"{\n"
"  f();\n"
Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -662,7 +662,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code


Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1129,7 +1129,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1141,7 +1141,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
+++ 

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 134641.
Typz added a comment.

Fix bug which was lingering: prevent inlining of ObjC special control blocks.


Repository:
  rC Clang

https://reviews.llvm.org/D43232

Files:
  include/clang/Format/Format.h
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -178,7 +178,8 @@
"@autoreleasepool {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@autoreleasepool\n"
"{\n"
"  f();\n"
@@ -196,7 +197,8 @@
"@synchronized(self) {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@synchronized(self)\n"
"{\n"
"  f();\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1127,7 +1127,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -314,6 +314,14 @@
   }
   return MergedLines;
 }
+// Don't merge block with left brace wrapped after ObjC special blocks
+if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
+I[-1]->First->is(tok::at) && I[-1]->First->Next) {
+  tok::ObjCKeywordKind kwId = I[-1]->First->Next->Tok.getObjCKeywordID();
+  if (kwId == clang::tok::objc_autoreleasepool ||
+  kwId == clang::tok::objc_synchronized)
+return 0;
+}
 // Try to merge a block with left brace wrapped that wasn't yet covered
 if (TheLine->Last->is(tok::l_brace)) {
   return !Style.BraceWrapping.AfterFunction ||
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -651,7 +651,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -178,7 +178,8 @@
"@autoreleasepool {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@autoreleasepool\n"
"{\n"
"  f();\n"
@@ -196,7 +197,8 @@
"@synchronized(self) {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@synchronized(self)\n"
"{\n"
"  f();\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz marked an inline comment as done.
Typz added inline comments.



Comment at: unittests/Format/FormatTestObjC.cpp:193-198
+  verifyFormat("@synchronized(self) {\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self) {\n"
+   "  f();\n"
+   "}\n");

benhamilton wrote:
> Why is the block repeated twice?
not sure why, I just copied the @autoreleasepool test code.
this may be to detect some more 'internal' error, which would lead to any 
visible effect on the first statement itself...


Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I'll try to fix this.

Can you please review D43114  in the mean 
time? This patch depends on it, so it must be merged first.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

A user can create an error by reasoning like this, after the code has been 
formatted this way (a long time ago) : "oh, I need to make an extra function 
call, but in all cases ah, here is the end of the switch, let's put my 
function call here".

I am not saying it happens often, I am just saying it breaks indentation : i.e. 
that two nested blocks should not close at the same depth. Breaking such things 
makes code harder to read/understand, and when you don't properly get the code 
you can more easily make a mistake. Obviously it should be caught in testing, 
but it is not always.

That said, I am not trying to say in any way that my approach is better. I 
think both `CaseBlockIndent = Block` or your variant (with the extra line break 
before opening brace; but applying it all the time) will indeed be consistent 
with the code and not break indentation; keeping the current behavior when 
`IndentCaseLabels = true` is also not a problem IMHO.

> And to me (but this is obviously objective), your suggestions hide the 
> structure of the code even more as they lead to a state where the closing 
> brace is not aligned with the start of the line/statement that contains the 
> opening braces. That looks like a bug to me more than anything else and I 
> don't think there is another situation where clang-format would do that.

The exact same thing happens for functions blocks, class blocks and control 
statements, depending on BraceWrapping modes. Actually, IMO wrapping the 
opening brace should probably be done according to 
`BraceWrapping.AfterControlStatement` flag.

  // BraceWrapping.AfterControlStatement = false
  switch (x) {
  case 0: {
  foo();
  break;
}
  }
  // BraceWrapping.AfterControlStatement = true
  switch (x)
  {
  case 0:
{
  foo();
  break;
}
  }

But I agree the `CaseBlockIndent = ClosingBrace` mode is definitely not 
consistent with the code. I think it is clearer this way, but that is 
definitely my own subjective opinion: in this case, I accept to lose the extra 
indent for the sake of compactness and to somehow mimic the "regular" case 
blocks (e.g. which do not include an actual block), but that is indeed really 
my personnal way to read code.


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D43232#1008004, @benhamilton wrote:

> Thanks! Can you add a test for this, please?


There are actually tests, though they are performed by activating Allman brace 
wrapping mode.
I am trying to change them, but this highlights another issue: when only 
`AfterControlStatement` is set, the block gets inlined, like this:

  @autoreleasepool
  { f(); }

This used to happen before as well, and would require fixing 
UnwrappedLineFormatter.cpp to understand that these blocks are linked to the 
previous ObjC keyword.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D43183#1008632, @djasper wrote:

> Yes, that's what I mean. What do you mean, the style is too error prone?


When `IndentCaseLabels` is false, the code gets formatted in a way that "hides" 
the structure of the code, by indenting the end of the case in the exact same 
way as the end of a switch, which is error prone IMHO.
Indentation should "highlight" the structure of the code, to make it easier to 
understand, and thus avoid this kind of confusion.


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 134411.
Typz marked an inline comment as done.
Typz added a comment.

Address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D43290

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
@@ -6867,6 +6885,14 @@
" 3, cc};",
getLLVMStyleWithColumns(60));
 
+  // Do not break between equal and opening brace.
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyleWithColumns(43);
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  AvoidBreakingFirstArgument.ColumnLimit = 43;
+  verifyFormat("vector aa = {\n"
+   "1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};",
+   AvoidBreakingFirstArgument);
+
   // Trailing commas.
   verifyFormat("vector x = {\n"
"1, 1, 1, 1, 1, 1, 1, 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal))
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
@@ -6867,6 +6885,14 @@
" 3, cc};",
getLLVMStyleWithColumns(60));
 
+  // Do not break between equal and opening brace.
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyleWithColumns(43);

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Tweaking the penalty handling would still be needed in any case, since the 
problem happens also when Cpp11BracedListStyle is true (though the effect is 
more subtle)

Generally, I think it is better to just rely on penalties, since it gives a way 
to compare and weight each solution. Then each style can decide what should 
break first: e.g. a style may also have a lower `PenaltyBreakAssignment`, thus 
wrapping after the equal sign would be expected...


Repository:
  rC Clang

https://reviews.llvm.org/D43290



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

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

This patch changes the behavior of PenaltyBreakBeforeFirstCallParameter
so that is does not apply when the brace comes after an assignment.

This way, variable initialization is wrapped more like an initializer
than like a function call, which seems more consistent with user
expectations.

With PenaltyBreakBeforeFirstCallParameter=200, this gives the following
code: (with Cpp11BracedListStyle=false)

Before :

  const std::unordered_map Something::MyHashTable =
  { { "a", 0 },
{ "b", 1 },
{ "c", 2 } };

After :

  const std::unordered_set Something::MyUnorderedSet = {
{ "a", 0 },
{ "b", 1 },
{ "c", 2 }
  };


Repository:
  rC Clang

https://reviews.llvm.org/D43290

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyle();
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   avoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
@@ -6867,6 +6885,14 @@
" 3, cc};",
getLLVMStyleWithColumns(60));
 
+  // Do not break between equal and opening brace.
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyleWithColumns(43);
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  avoidBreakingFirstArgument.ColumnLimit = 43;
+  verifyFormat("vector aa = {\n"
+   "1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};",
+   avoidBreakingFirstArgument);
+
   // Trailing commas.
   verifyFormat("vector x = {\n"
"1, 1, 1, 1, 1, 1, 1, 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal))
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyle();
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   avoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Hum, not sure I fully got your proposal. So you actually mean that clang-format 
you format like this (with 4-space indent for clarity):

  switch (x) {
  case 0:
  break;
  case 1: {
  foo();
  break;
  }
  case 2: {
  foo();
  } break;
  case 3:
  {
  foo();
  }
  bar();
  break;
  }

Is this right?

If so it does not really help me: I don't care so much how it is formatted, but 
I think the current way is way too error prone (and I cannot change the style 
to indent the case blocks themself). So i'll have to keep a patch in my fork :-(
Or maybe the behavior should be dependant on `IndentCaseLabels` (though this 
would change LLVM style formatting) ?


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

It is explicitly documented in google style guide: 
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements :

> case blocks in switch statements can have curly braces or not, depending on 
> your preference. If you do include curly braces they should be placed as 
> shown below.
> 
> If not conditional on an enumerated value, switch statements should always 
> have a default case (in the case of an enumerated value, the compiler will 
> warn you if any values are not handled). If the default case should never 
> execute, simply assert:
> 
>   switch (var) {
> case 0: {  // 2 space indent
>   ...  // 4 space indent
>   break;
> }
> case 1: {
>   ...
>   break;
> }
> default: {
>   assert(false);
> }
>   }

So IMHO we cannot just change the current (or default) behaviour.


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> We'll just format those cases in a somewhat weird way and users can either 
> accept that or change their code to not need it.

I think we have a really diverging opinion on this. From my experience, people 
will mostly accept the output of the formatter without question : therefore I 
think we should strive to have the formatter format things as "correctly" as 
possible. And looking at the existing options and various complex formatting 
algorithms implemented I think this reasoning has been applied to some extent 
:-)
So I personnally would be willing to add some code/options even for such kind 
of corner cases; but then I am not the one maintaining the clang-format project.

> I don't particularly care which of these options we go with, but I would be 
> really hesitant to introduce an style flag for it. This is so rare, that 
> almost no one needs this flag (or should need this flag). Thus, the cost of 
> the flag (discoverability of flags for users, increased maintenance cost, 
> etc.) strongly outweigh the benefit.

Any change will still require a new flag somewhere, since the currently 
implemented behavior is :
1- the documented way to format in Google style
2- the expected format in LLVM style, according to the clang-format unit test

It should thus probably not be changed.

> IMO, for the same reason, this specific issue will not become part of the 
> style guide of projects.

I definitely agree on this, but it is actually part of some styles (e.g. 
Google's)

> If you want to change something, I'd vote for making clang fall back to this 
> behavior:
> 
>   case A:
> {
>   stuff();
> }
> moreStuff();
> break;
>   }
>
> 
> i.e. not let it put the "{" on the same line as the "case ..." if there is a 
> trailing statement (other than "break;" maybe).

I am fine with that formatting (though I did not implement it since it requires 
tweaking the code in more places, instead of essentially modifying a single 
function like I did).


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz marked 4 inline comments as done.
Typz added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();

benhamilton wrote:
> Typz wrote:
> > benhamilton wrote:
> > > Typz wrote:
> > > > Wondering if formatting with this style is appropriate: the 
> > > > clang-format doc points in that direction, but it seems to me both 
> > > > `@synchronized` and `@autoreleasepool` are more akin to "control 
> > > > statements".
> > > > 
> > > > For consistency (esp. in ObjC++ code), as a user, I would tend to have 
> > > > these blocks indented like control statements while 
> > > > interfaces/implementations blocks would be indented like 
> > > > classes/structs.
> > > > 
> > > > So maybe it would be better to introduce a new BraceWrapping style 
> > > > 'AfterObjCSpecialBlock` to control these cases, matching the 
> > > > possibilities that are given for control statements vs classes. What do 
> > > > you think?
> > > Hmm, I definitely agree with that logic. I don't see them acting as 
> > > declarations in any way, they are definitely like control statements.
> > > 
> > Ok, i can change this. Two questions though:
> > * Should I do this in this patch, or a separate patch? (won't be a big 
> > change in any case, but it can still be seen as 2 separate issues/changes)
> > * Should I introduce a new BraceWrapping flag (`AfterObjCSpecialBlock`), or 
> > simply apply `AfterControlStatement` to these blocks?
> Personally, I feel `AfterControlStatement` applies to these. I'm not an 
> expert on this, though, so I will defer to others on the review.
I created another diff to change the field that is used to control brace 
wrapping after these blocks: https://reviews.llvm.org/D43232

This patch here should now only relate to the parsing of `@synchronized` blocks.


Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

This is done as discussed in https://reviews.llvm.org/D43114.
But I can instead add a new `AfterObjCSpecialBlock` brace wrapping flag.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek, benhamilton.

ObjC defines `@autoreleasepool` and `@synchronized` control blocks. These
used to be formatted according to the `AfterObjCDeclaration` brace-
wrapping flag, which is not very consistent.

This patch changes the behavior to use the `AfterControlStatement` flag
instead. This should not affect the behavior unless a custom brace
wrapping mode is used.


Repository:
  rC Clang

https://reviews.llvm.org/D43232

Files:
  include/clang/Format/Format.h
  lib/Format/UnwrappedLineParser.cpp


Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1127,7 +1127,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -651,7 +651,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code


Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1127,7 +1127,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -651,7 +651,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Do you have a reference to style guides recommending any of this?

Unfortunately not... I think this is a really advanced topic, and often not 
recommended way to format code at all (e.g. "if you need a block, you may as 
well use a function"). I can find the current implementation documented in 
google style guide, where there is an extra indent inside switch [e.g. 
`IndentCaseLabels`], which alleviates the ambiguity. But it is not documented 
(or shown) in the LLVM coding style, where it makes the code IMO really 
difficult to follow.

I believe the 2 extra modes I introduce should provide for most cases, leaving 
the decision to user:

- `None` mode matches google-style, and is perfectly fine when 
`IndentCaseLabels == true`
- `ClosingBrace` mode is "logical" with what people may want (e.g. have a scope 
for the whole `case` block), but is not syntaxically correct and may thus 
mislead
- `Block` mode is syntaxically correct, but somewhat breaks the switch layout 
by moving break to different indentation levels depending on the use of braces

Which mode is use is then left to the user, depending on the "goals" of their 
coding style.

Also, maybe this is a small-enough detail that it would be better moved to the 
`BraceWrapping` field, to allow finely tuning the expected behavior (e.g. in 
Custom brace mode) while keeping the 'main' options simple (even though it is 
not technically a way to wrap the braces).


Repository:
  rC Clang

https://reviews.llvm.org/D43183



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

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

When a block is started after a case label, clang-format does add extra
indent to the content of this block: the block content is indented one
level (with respect to the switch) while the closing brace is not
indented, and closes at switch level.

This gives the following code:

  switch (x) {
  case A: {
stuff();
  } break;
  }

This makes it easy to confuse the closing brace of the 'case' with that
the whole 'switch', especially if more code is added after the brace:

  switch (x) {
  case A: {
stuff();
  }
moreStuff();
break;
  }

This patch introduces a new CaseBlockIndent switch, which provides
alternative formatting for these cases:

- `CaseBlockIndent = None` : default behavior, same behavior as before

- `CaseBlockIndent = ClosingBrace` : indent the closing brace to the

same level as its content.

  switch (x) {
  case A: {
stuff();
} break;
  }

- `CaseBlockIndent = Block` : add an extra level of indent for the

content of the block.

  switch (x) {
  case A: {
  stuff();
} break;
  }


Repository:
  rC Clang

https://reviews.llvm.org/D43183

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -980,6 +980,96 @@
"  return;\n"
"}",
getLLVMStyleWithColumns(34));
+
+  FormatStyle indentClosingBrace = getLLVMStyle();
+  indentClosingBrace.CaseBlockIndent = FormatStyle::CBIS_ClosingBrace;
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "  f();\n"
+   "  break;\n"
+   "}",
+   indentClosingBrace);
+  verifyFormat("switch (x) {\n"
+   "case 1: {\n"
+   "  f();\n"
+   "  break;\n"
+   "  }\n"
+   "case 2: {\n"
+   "  break;\n"
+   "  }\n"
+   "}",
+   indentClosingBrace);
+  verifyFormat("switch (x) {\n"
+   "case 1: {\n"
+   "  f();\n"
+   "  } break;\n"
+   "case 2: {\n"
+   "  } break;\n"
+   "}",
+   indentClosingBrace);
+  verifyFormat("switch (x) {\n"
+   "case 1: {\n"
+   "  f();\n"
+   "  }\n"
+   "  g();\n"
+   "  break;\n"
+   "}",
+   indentClosingBrace);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "  f();\n"
+   "  {\n"
+   "g();\n"
+   "h();\n"
+   "  }\n"
+   "  break;\n"
+   "}",
+   indentClosingBrace);
+
+  FormatStyle indentBlock = getLLVMStyle();
+  indentBlock.CaseBlockIndent = FormatStyle::CBIS_Block;
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "  f();\n"
+   "  break;\n"
+   "}",
+   indentBlock);
+  verifyFormat("switch (x) {\n"
+   "case 1: {\n"
+   "f();\n"
+   "break;\n"
+   "  }\n"
+   "case 2: {\n"
+   "break;\n"
+   "  }\n"
+   "}",
+   indentBlock);
+  verifyFormat("switch (x) {\n"
+   "case 1: {\n"
+   "f();\n"
+   "  } break;\n"
+   "case 2: {\n"
+   "  } break;\n"
+   "}",
+   indentBlock);
+  verifyFormat("switch (x) {\n"
+   "case 1: {\n"
+   "f();\n"
+   "  }\n"
+   "  g();\n"
+   "  break;\n"
+   "}",
+   indentBlock);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "  f();\n"
+   "  {\n"
+   "g();\n"
+   "h();\n"
+   "  }\n"
+   "  break;\n"
+   "}",
+   indentBlock);
 }
 
 TEST_F(FormatTest, CaseRanges) {
@@ -10413,6 +10503,13 @@
   CHECK_PARSE("AllowShortFunctionsOnASingleLine: true",
   AllowShortFunctionsOnASingleLine, FormatStyle::SFS_All);
 
+  Style.CaseBlockIndent = FormatStyle::CBIS_Block;
+  CHECK_PARSE("CaseBlockIndent: None", CaseBlockIndent, FormatStyle::CBIS_None);
+  CHECK_PARSE("CaseBlockIndent: ClosingBrace", CaseBlockIndent,
+  FormatStyle::CBIS_ClosingBrace);
+  CHECK_PARSE("CaseBlockIndent: Block", CaseBlockIndent,
+  FormatStyle::CBIS_Block);
+
   Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
   CHECK_PARSE("SpaceBeforeParens: Never", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: 

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();

benhamilton wrote:
> Typz wrote:
> > Wondering if formatting with this style is appropriate: the clang-format 
> > doc points in that direction, but it seems to me both `@synchronized` and 
> > `@autoreleasepool` are more akin to "control statements".
> > 
> > For consistency (esp. in ObjC++ code), as a user, I would tend to have 
> > these blocks indented like control statements while 
> > interfaces/implementations blocks would be indented like classes/structs.
> > 
> > So maybe it would be better to introduce a new BraceWrapping style 
> > 'AfterObjCSpecialBlock` to control these cases, matching the possibilities 
> > that are given for control statements vs classes. What do you think?
> Hmm, I definitely agree with that logic. I don't see them acting as 
> declarations in any way, they are definitely like control statements.
> 
Ok, i can change this. Two questions though:
* Should I do this in this patch, or a separate patch? (won't be a big change 
in any case, but it can still be seen as 2 separate issues/changes)
* Should I introduce a new BraceWrapping flag (`AfterObjCSpecialBlock`), or 
simply apply `AfterControlStatement` to these blocks?


Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC324741: clang-format: keep ObjC colon alignment with short 
object name (authored by Typz, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43121?vs=133619=133620#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43121

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -693,8 +693,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   // Inline block as a first argument.
   verifyFormat("[object justBlock:^{\n"
@@ -760,6 +760,26 @@
   "  backing:NSBackingStoreBuffered\n"
   "defer:NO]);\n"
   "}");
+
+  // Respect continuation indent and colon alignment (e.g. when object name is
+  // short, and first selector is the longest one)
+  Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ContinuationIndentWidth = 8;
+  verifyFormat("[self performSelectorOnMainThread:@selector(loadAccessories)\n"
+   "   withObject:nil\n"
+   "waitUntilDone:false];");
+  verifyFormat("[self performSelector:@selector(loadAccessories)\n"
+   "withObjectOnMainThread:nil\n"
+   " waitUntilDone:false];");
+  verifyFormat("[a\n"
+   "
performSelectorOnMainThread:@selector(loadAccessories)\n"
+   " withObject:nil\n"
+   "  waitUntilDone:false];");
+  verifyFormat("[self // force wrapping\n"
+   "
performSelectorOnMainThread:@selector(loadAccessories)\n"
+   " withObject:nil\n"
+   "  waitUntilDone:false];");
 }
 
 TEST_F(FormatTestObjC, ObjCAt) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -591,12 +591,12 @@
 BeforePrevious->is(tok::r_square) ||
 Contexts.back().LongestObjCSelectorName == 0) {
   Tok->Previous->Type = TT_SelectorName;
-  if (Tok->Previous->ColumnWidth >
-  Contexts.back().LongestObjCSelectorName)
-Contexts.back().LongestObjCSelectorName =
-Tok->Previous->ColumnWidth;
   if (!Contexts.back().FirstObjCSelectorName)
 Contexts.back().FirstObjCSelectorName = Tok->Previous;
+  else if (Tok->Previous->ColumnWidth >
+   Contexts.back().LongestObjCSelectorName)
+Contexts.back().LongestObjCSelectorName =
+Tok->Previous->ColumnWidth;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -701,7 +701,8 @@
  ? std::max(State.Stack.back().Indent,
 State.FirstIndent + Style.ContinuationIndentWidth)
  : State.Stack.back().Indent) +
-NextNonComment->LongestObjCSelectorName;
+std::max(NextNonComment->LongestObjCSelectorName,
+ NextNonComment->ColumnWidth);
   }
 } else if (State.Stack.back().AlignColons &&
State.Stack.back().ColonPos <= NextNonComment->ColumnWidth) {
@@ -900,7 +901,8 @@
   ? std::max(State.Stack.back().Indent,
  State.FirstIndent + Style.ContinuationIndentWidth)
   : State.Stack.back().Indent) +
- NextNonComment->LongestObjCSelectorName -
+ std::max(NextNonComment->LongestObjCSelectorName,
+  NextNonComment->ColumnWidth) -
  NextNonComment->ColumnWidth;
 }
 if (!State.Stack.back().AlignColons)


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -693,8 +693,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 133619.
Typz added a comment.

rebase on latest master.


Repository:
  rC Clang

https://reviews.llvm.org/D43121

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -693,8 +693,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   // Inline block as a first argument.
   verifyFormat("[object justBlock:^{\n"
@@ -760,6 +760,26 @@
   "  backing:NSBackingStoreBuffered\n"
   "defer:NO]);\n"
   "}");
+
+  // Respect continuation indent and colon alignment (e.g. when object name is
+  // short, and first selector is the longest one)
+  Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ContinuationIndentWidth = 8;
+  verifyFormat("[self performSelectorOnMainThread:@selector(loadAccessories)\n"
+   "   withObject:nil\n"
+   "waitUntilDone:false];");
+  verifyFormat("[self performSelector:@selector(loadAccessories)\n"
+   "withObjectOnMainThread:nil\n"
+   " waitUntilDone:false];");
+  verifyFormat("[a\n"
+   "
performSelectorOnMainThread:@selector(loadAccessories)\n"
+   " withObject:nil\n"
+   "  waitUntilDone:false];");
+  verifyFormat("[self // force wrapping\n"
+   "
performSelectorOnMainThread:@selector(loadAccessories)\n"
+   " withObject:nil\n"
+   "  waitUntilDone:false];");
 }
 
 TEST_F(FormatTestObjC, ObjCAt) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -591,12 +591,12 @@
 BeforePrevious->is(tok::r_square) ||
 Contexts.back().LongestObjCSelectorName == 0) {
   Tok->Previous->Type = TT_SelectorName;
-  if (Tok->Previous->ColumnWidth >
-  Contexts.back().LongestObjCSelectorName)
-Contexts.back().LongestObjCSelectorName =
-Tok->Previous->ColumnWidth;
   if (!Contexts.back().FirstObjCSelectorName)
 Contexts.back().FirstObjCSelectorName = Tok->Previous;
+  else if (Tok->Previous->ColumnWidth >
+   Contexts.back().LongestObjCSelectorName)
+Contexts.back().LongestObjCSelectorName =
+Tok->Previous->ColumnWidth;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -701,7 +701,8 @@
  ? std::max(State.Stack.back().Indent,
 State.FirstIndent + Style.ContinuationIndentWidth)
  : State.Stack.back().Indent) +
-NextNonComment->LongestObjCSelectorName;
+std::max(NextNonComment->LongestObjCSelectorName,
+ NextNonComment->ColumnWidth);
   }
 } else if (State.Stack.back().AlignColons &&
State.Stack.back().ColonPos <= NextNonComment->ColumnWidth) {
@@ -900,7 +901,8 @@
   ? std::max(State.Stack.back().Indent,
  State.FirstIndent + Style.ContinuationIndentWidth)
   : State.Stack.back().Indent) +
- NextNonComment->LongestObjCSelectorName -
+ std::max(NextNonComment->LongestObjCSelectorName,
+  NextNonComment->ColumnWidth) -
  NextNonComment->ColumnWidth;
 }
 if (!State.Stack.back().AlignColons)


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -693,8 +693,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   // Inline block as a first argument.
   verifyFormat("[object justBlock:^{\n"
@@ -760,6 +760,26 

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:900
+ std::max(NextNonComment->LongestObjCSelectorName,
+  unsigned(NextNonComment->TokenText.size())) -
  NextNonComment->ColumnWidth;

djasper wrote:
> I'd prefer to use std::max( .. )
> 
> (and we generally don't use c-style casts)
this is C++-style C-style cast :-)

anyway changed to use ColumnWidth, as this is also what is used for 
initializing LongestObjCSelectorName.


Repository:
  rC Clang

https://reviews.llvm.org/D43121



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

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 133596.
Typz marked 2 inline comments as done.
Typz added a comment.

Address review comments


Repository:
  rC Clang

https://reviews.llvm.org/D43121

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -652,8 +652,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   Style.ColumnLimit = 70;
   verifyFormat(
@@ -686,6 +686,26 @@
   "  backing:NSBackingStoreBuffered\n"
   "defer:NO]);\n"
   "}");
+
+  // Respect continuation indent and colon alignment (e.g. when object name is
+  // short, and first selector is the longest one)
+  Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ContinuationIndentWidth = 8;
+  verifyFormat("[self performSelectorOnMainThread:@selector(loadAccessories)\n"
+   "   withObject:nil\n"
+   "waitUntilDone:false];");
+  verifyFormat("[self performSelector:@selector(loadAccessories)\n"
+   "withObjectOnMainThread:nil\n"
+   " waitUntilDone:false];");
+  verifyFormat("[a\n"
+   "
performSelectorOnMainThread:@selector(loadAccessories)\n"
+   " withObject:nil\n"
+   "  waitUntilDone:false];");
+  verifyFormat("[self // force wrapping\n"
+   "
performSelectorOnMainThread:@selector(loadAccessories)\n"
+   " withObject:nil\n"
+   "  waitUntilDone:false];");
 }
 
 TEST_F(FormatTestObjC, ObjCAt) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -571,12 +571,12 @@
 BeforePrevious->is(tok::r_square) ||
 Contexts.back().LongestObjCSelectorName == 0) {
   Tok->Previous->Type = TT_SelectorName;
-  if (Tok->Previous->ColumnWidth >
-  Contexts.back().LongestObjCSelectorName)
-Contexts.back().LongestObjCSelectorName =
-Tok->Previous->ColumnWidth;
   if (!Contexts.back().FirstObjCSelectorName)
 Contexts.back().FirstObjCSelectorName = Tok->Previous;
+  else if (Tok->Previous->ColumnWidth >
+   Contexts.back().LongestObjCSelectorName)
+Contexts.back().LongestObjCSelectorName =
+Tok->Previous->ColumnWidth;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -696,7 +696,8 @@
  ? std::max(State.Stack.back().Indent,
 State.FirstIndent + Style.ContinuationIndentWidth)
  : State.Stack.back().Indent) +
-NextNonComment->LongestObjCSelectorName;
+std::max(NextNonComment->LongestObjCSelectorName,
+ NextNonComment->ColumnWidth);
   }
 } else if (State.Stack.back().AlignColons &&
State.Stack.back().ColonPos <= NextNonComment->ColumnWidth) {
@@ -895,7 +896,8 @@
   ? std::max(State.Stack.back().Indent,
  State.FirstIndent + Style.ContinuationIndentWidth)
   : State.Stack.back().Indent) +
- NextNonComment->LongestObjCSelectorName -
+ std::max(NextNonComment->LongestObjCSelectorName,
+  NextNonComment->ColumnWidth) -
  NextNonComment->ColumnWidth;
 }
 if (!State.Stack.back().AlignColons)


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -652,8 +652,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   Style.ColumnLimit = 70;
   verifyFormat(
@@ -686,6 +686,26 @@
   "  

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 133589.
Typz added a comment.

fix type in commit message


Repository:
  rC Clang

https://reviews.llvm.org/D43121

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -652,8 +652,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   Style.ColumnLimit = 70;
   verifyFormat(
@@ -686,7 +686,19 @@
   "  backing:NSBackingStoreBuffered\n"
   "defer:NO]);\n"
   "}");
-}
+
+  // Respect continuation indent and colon alignment (e.g. when object name is
+  // short, and first selector is the longest one)
+  Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ContinuationIndentWidth = 8;
+  verifyFormat("[self performSelectorOnMainThread:@selector(loadAccessories)\n"
+   "   withObject:nil\n"
+   "waitUntilDone:false];");
+  verifyFormat("[self performSelector:@selector(loadAccessories)\n"
+   "withObjectOnMainThread:nil\n"
+   " waitUntilDone:false];");
+  }
 
 TEST_F(FormatTestObjC, ObjCAt) {
   verifyFormat("@autoreleasepool");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -571,12 +571,12 @@
 BeforePrevious->is(tok::r_square) ||
 Contexts.back().LongestObjCSelectorName == 0) {
   Tok->Previous->Type = TT_SelectorName;
-  if (Tok->Previous->ColumnWidth >
-  Contexts.back().LongestObjCSelectorName)
-Contexts.back().LongestObjCSelectorName =
-Tok->Previous->ColumnWidth;
   if (!Contexts.back().FirstObjCSelectorName)
 Contexts.back().FirstObjCSelectorName = Tok->Previous;
+  else if (Tok->Previous->ColumnWidth >
+   Contexts.back().LongestObjCSelectorName)
+Contexts.back().LongestObjCSelectorName =
+Tok->Previous->ColumnWidth;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -696,7 +696,8 @@
  ? std::max(State.Stack.back().Indent,
 State.FirstIndent + Style.ContinuationIndentWidth)
  : State.Stack.back().Indent) +
-NextNonComment->LongestObjCSelectorName;
+std::max(NextNonComment->LongestObjCSelectorName,
+ unsigned(NextNonComment->TokenText.size()));
   }
 } else if (State.Stack.back().AlignColons &&
State.Stack.back().ColonPos <= NextNonComment->ColumnWidth) {
@@ -895,7 +896,8 @@
   ? std::max(State.Stack.back().Indent,
  State.FirstIndent + Style.ContinuationIndentWidth)
   : State.Stack.back().Indent) +
- NextNonComment->LongestObjCSelectorName -
+ std::max(NextNonComment->LongestObjCSelectorName,
+  unsigned(NextNonComment->TokenText.size())) -
  NextNonComment->ColumnWidth;
 }
 if (!State.Stack.back().AlignColons)


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -652,8 +652,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   Style.ColumnLimit = 70;
   verifyFormat(
@@ -686,7 +686,19 @@
   "  backing:NSBackingStoreBuffered\n"
   "defer:NO]);\n"
   "}");
-}
+
+  // Respect continuation indent and colon alignment (e.g. when object name is
+  // short, and first selector is the longest one)
+  Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ContinuationIndentWidth = 8;
+  verifyFormat("[self performSelectorOnMainThread:@selector(loadAccessories)\n"
+   "   withObject:nil\n"
+ 

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

When the target object expression is short and the first selector name is
long, clang-format used to break the colon alignment:

  [I performSelectorOnMainThread:@selector(loadAccessories)
   withObject:nil
waitUntilDone:false];

This happens because the colon is placed at `ContinuationIndent +
LongestObjCSelectorName`, so that any selector can be wrapped. This is
however not needed in case the longest selector is the first one, and not
wrapped.

To overcome this, this patch does not include the first selector in
`LongestObjCSelectorName` computation (in TokenAnnotator), and lets
ContinuationIndenter account decide how to account for the first selector
when wrapping. (Note this was already partly the case, see line 521 of
ContinuationIndenter.cpp)

This way, the code gets properly aligned whenever possible without
breaking the continuation indent.

  [I performSelectorOnMainThread:@selector(loadAccessories)
  withObject:nil
   waitUntilDone:false];
  [I // force break
  performSelectorOnMainThread:@selector(loadAccessories)
   withObject:nil
waitUntilDone:false];
  [I perform:@selector(loadAccessories)
  withSelectorOnMainThread:true
 waitUntilDone:false];


Repository:
  rC Clang

https://reviews.llvm.org/D43121

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -652,8 +652,8 @@
   // Formats pair-parameters.
   verifyFormat("[I drawRectOn:surface ofSize:aa:bbb atOrigin:cc:dd];");
   verifyFormat("[I drawRectOn:surface //\n"
-   "ofSize:aa:bbb\n"
-   "  atOrigin:cc:dd];");
+   "   ofSize:aa:bbb\n"
+   " atOrigin:cc:dd];");
 
   Style.ColumnLimit = 70;
   verifyFormat(
@@ -686,7 +686,19 @@
   "  backing:NSBackingStoreBuffered\n"
   "defer:NO]);\n"
   "}");
-}
+
+  // Respect continuation indent and colon alignment (e.g. when object name is
+  // short, and first selector is the longest one)
+  Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ContinuationIndentWidth = 8;
+  verifyFormat("[self performSelectorOnMainThread:@selector(loadAccessories)\n"
+   "   withObject:nil\n"
+   "waitUntilDone:false];");
+  verifyFormat("[self performSelector:@selector(loadAccessories)\n"
+   "withObjectOnMainThread:nil\n"
+   " waitUntilDone:false];");
+  }
 
 TEST_F(FormatTestObjC, ObjCAt) {
   verifyFormat("@autoreleasepool");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -571,12 +571,12 @@
 BeforePrevious->is(tok::r_square) ||
 Contexts.back().LongestObjCSelectorName == 0) {
   Tok->Previous->Type = TT_SelectorName;
-  if (Tok->Previous->ColumnWidth >
-  Contexts.back().LongestObjCSelectorName)
-Contexts.back().LongestObjCSelectorName =
-Tok->Previous->ColumnWidth;
   if (!Contexts.back().FirstObjCSelectorName)
 Contexts.back().FirstObjCSelectorName = Tok->Previous;
+  else if (Tok->Previous->ColumnWidth >
+   Contexts.back().LongestObjCSelectorName)
+Contexts.back().LongestObjCSelectorName =
+Tok->Previous->ColumnWidth;
 }
   } else if (Contexts.back().ColonIsForRangeExpr) {
 Tok->Type = TT_RangeBasedForLoopColon;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -696,7 +696,8 @@
  ? std::max(State.Stack.back().Indent,
 State.FirstIndent + Style.ContinuationIndentWidth)
  : State.Stack.back().Indent) +
-NextNonComment->LongestObjCSelectorName;
+std::max(NextNonComment->LongestObjCSelectorName,
+ unsigned(NextNonComment->TokenText.size()));
   }
 } else if (State.Stack.back().AlignColons &&
State.Stack.back().ColonPos <= NextNonComment->ColumnWidth) {
@@ -895,7 +896,8 @@
   ? std::max(State.Stack.back().Indent,
  State.FirstIndent + Style.ContinuationIndentWidth)
   : State.Stack.back().Indent) +
- NextNonComment->LongestObjCSelectorName -
+ 

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D42787#1000687, @krasimir wrote:

> We could adapt the single-argument version instead, turning:
>
>   foo(bb +
>   c);
>
>
> into:
>
>   foo(bb +
>   c);
>


We could indeed, but that would probably make neither djasper or me happy :-)


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D42684



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();

Wondering if formatting with this style is appropriate: the clang-format doc 
points in that direction, but it seems to me both `@synchronized` and 
`@autoreleasepool` are more akin to "control statements".

For consistency (esp. in ObjC++ code), as a user, I would tend to have these 
blocks indented like control statements while interfaces/implementations blocks 
would be indented like classes/structs.

So maybe it would be better to introduce a new BraceWrapping style 
'AfterObjCSpecialBlock` to control these cases, matching the possibilities that 
are given for control statements vs classes. What do you think?


Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

The blocks used to be formatted using the "default" behavior, and would
thus be mistaken for function calls followed by blocks: this could lead
to unexpected inlining of the block and extra line-break before the
opening brace.

They are now formatted similarly to `@autoreleasepool` blocks, as
expected:

  @synchronized(self) {
  f();
  }


Repository:
  rC Clang

https://reviews.llvm.org/D43114

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,24 @@
"}\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+  verifyFormat("@synchronized(self) {\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self) {\n"
+   "  f();\n"
+   "}\n");
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  verifyFormat("@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1121,6 +1121,18 @@
 }
 addUnwrappedLine();
 return;
+  case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+   // Skip synchronization object
+   parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+  parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
   case tok::objc_try:
 // This branch isn't strictly necessary (the kw_try case below would
 // do this too after the tok::at is parsed above).  But be explicit.


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,24 @@
"}\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+  verifyFormat("@synchronized(self) {\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self) {\n"
+   "  f();\n"
+   "}\n");
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  verifyFormat("@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1121,6 +1121,18 @@
 }
 addUnwrappedLine();
 return;
+  case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+   // Skip synchronization object
+   parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+  parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
   case tok::objc_try:
 // This branch isn't strictly necessary (the kw_try case below would
 // do this too after the tok::at is parsed above).  But be explicit.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 133204.
Typz added a comment.

Fix indentation of inheritance list, which is actually based on 
`ConstructorInitializerIndentWidth`.

Maybe a new option (`InheritanceListIndentWidth`) should be used instead?


Repository:
  rC Clang

https://reviews.llvm.org/D43015

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1286,15 +1286,40 @@
   verifyFormat("class ::A::B {};");
 }
 
-TEST_F(FormatTest, BreakBeforeInheritanceComma) {
-  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
-  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
-
-  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+TEST_F(FormatTest, BreakInheritanceStyle) {
+  FormatStyle StyleWithInheritanceBreakBeforeComma = getLLVMStyle();
+  StyleWithInheritanceBreakBeforeComma.BreakInheritanceList =
+  FormatStyle::BILS_BeforeComma;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakBeforeComma);
   verifyFormat("class MyClass\n"
": public X\n"
", public Y {};",
-   StyleWithInheritanceBreak);
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("class AA\n"
+   ": public BB\n"
+   ", public CC {};",
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("struct a\n"
+   ": public aaa< // break\n"
+   "  > {};",
+   StyleWithInheritanceBreakBeforeComma);
+
+  FormatStyle StyleWithInheritanceBreakAfterColon = getLLVMStyle();
+  StyleWithInheritanceBreakAfterColon.BreakInheritanceList =
+  FormatStyle::BILS_AfterColon;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class MyClass : public X, public Y {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class AA :\n"
+   "public BB,\n"
+   "public CC {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("struct a :\n"
+   "public aaa< // break\n"
+   "> {};",
+   StyleWithInheritanceBreakAfterColon);
 }
 
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
@@ -3618,6 +3643,23 @@
"  aa,\n"
"  bb {}",
Style);
+
+  // `ConstructorInitializerIndentWidth` actually applies to InheritanceList as well
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
+  verifyFormat("class SomeClass\n"
+   "  : public aa,\n"
+   "public bb {};",
+   Style);
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeComma;
+  verifyFormat("class SomeClass\n"
+   "  : public aa\n"
+   "  , public bb {};",
+   Style);
+  Style.BreakInheritanceList = FormatStyle::BILS_AfterColon;
+  verifyFormat("class SomeClass :\n"
+   "  public aa,\n"
+   "  public bb {};",
+   Style);
 }
 
 #ifndef EXPENSIVE_CHECKS
@@ -10172,7 +10214,6 @@
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakStringLiterals);
-  CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
   CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
@@ -10284,6 +10325,17 @@
   CHECK_PARSE("BreakConstructorInitializersBeforeComma: true",
   BreakConstructorInitializers, FormatStyle::BCIS_BeforeComma);
 
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
+  CHECK_PARSE("BreakInheritanceList: BeforeComma",
+  BreakInheritanceList, FormatStyle::BILS_BeforeComma);
+  CHECK_PARSE("BreakInheritanceList: AfterColon",
+  BreakInheritanceList, FormatStyle::BILS_AfterColon);
+  CHECK_PARSE("BreakInheritanceList: BeforeColon",
+  BreakInheritanceList, 

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: djasper, krasimir, klimek.

This option replaces the BreakBeforeInheritanceComma option with an
enum, thus introducing a mode where the colon stays on the same line as
constructor declaration:

  // When it fits on line:
  class A : public B, public C {
...
  };
  
  // When it does not fit:
  class A :
  public B,
  public C {
...
  };

This matches the behavior of the `BreakConstructorInitializers` option,
introduced in https://reviews.llvm.org/D32479.


Repository:
  rC Clang

https://reviews.llvm.org/D43015

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1286,15 +1286,40 @@
   verifyFormat("class ::A::B {};");
 }
 
-TEST_F(FormatTest, BreakBeforeInheritanceComma) {
-  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
-  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
-
-  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+TEST_F(FormatTest, BreakInheritanceStyle) {
+  FormatStyle StyleWithInheritanceBreakBeforeComma = getLLVMStyle();
+  StyleWithInheritanceBreakBeforeComma.BreakInheritanceList =
+  FormatStyle::BILS_BeforeComma;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakBeforeComma);
   verifyFormat("class MyClass\n"
": public X\n"
", public Y {};",
-   StyleWithInheritanceBreak);
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("class AA\n"
+   ": public BB\n"
+   ", public CC {};",
+   StyleWithInheritanceBreakBeforeComma);
+  verifyFormat("struct a\n"
+   ": public aaa< // break\n"
+   "  > {};",
+   StyleWithInheritanceBreakBeforeComma);
+
+  FormatStyle StyleWithInheritanceBreakAfterColon = getLLVMStyle();
+  StyleWithInheritanceBreakAfterColon.BreakInheritanceList =
+  FormatStyle::BILS_AfterColon;
+  verifyFormat("class MyClass : public X {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class MyClass : public X, public Y {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("class AA :\n"
+   "public BB,\n"
+   "public CC {};",
+   StyleWithInheritanceBreakAfterColon);
+  verifyFormat("struct a :\n"
+   "public aaa< // break\n"
+   "> {};",
+   StyleWithInheritanceBreakAfterColon);
 }
 
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
@@ -10172,7 +10197,6 @@
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakStringLiterals);
-  CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
   CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
@@ -10284,6 +10308,17 @@
   CHECK_PARSE("BreakConstructorInitializersBeforeComma: true",
   BreakConstructorInitializers, FormatStyle::BCIS_BeforeComma);
 
+  Style.BreakInheritanceList = FormatStyle::BILS_BeforeColon;
+  CHECK_PARSE("BreakInheritanceList: BeforeComma",
+  BreakInheritanceList, FormatStyle::BILS_BeforeComma);
+  CHECK_PARSE("BreakInheritanceList: AfterColon",
+  BreakInheritanceList, FormatStyle::BILS_AfterColon);
+  CHECK_PARSE("BreakInheritanceList: BeforeColon",
+  BreakInheritanceList, FormatStyle::BILS_BeforeColon);
+  // For backward compatibility:
+  CHECK_PARSE("BreakBeforeInheritanceComma: true",
+  BreakInheritanceList, FormatStyle::BILS_BeforeComma);
+
   Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
   CHECK_PARSE("AlignAfterOpenBracket: Align", AlignAfterOpenBracket,
   FormatStyle::BAS_Align);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2652,7 +2652,8 @@
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
   // Break only if we have multiple inheritance.
-  if (Style.BreakBeforeInheritanceComma && Right.is(TT_InheritanceComma))
+  if (Style.BreakInheritanceList == FormatStyle::BILS_BeforeComma &&
+  Right.is(TT_InheritanceComma))
 return true;
   if (Right.is(tok::string_literal) && 

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Indeed, I have yet find more precisely documented coding rules which require 
this format, but I thought I could at least address the non-precise aspect of 
the patch itself in the mean-time.


https://reviews.llvm.org/D32525



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

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132963.
Typz added a comment.

Split the option into 3 separate options: SpaceBeforeCtorInitializerColon, 
SpaceBeforeInheritanceColon and SpaceBeforeRangeBasedForLoopColon.
This makes each option clearer and more consistent, with no ambiguities due to 
interractions with other options.


https://reviews.llvm.org/D32525

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7519,6 +7519,91 @@
   verifyFormat("a or_eq 8;", Spaces);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeColon) {
+  verifyFormat("class Foo : public Bar {};");
+  verifyFormat("Foo::Foo() : foo(1) {}");
+  verifyFormat("for (auto a : b) {\n}");
+  verifyFormat("int x = a ? b : c;");
+  verifyFormat("{\n"
+   "label0:\n"
+   "  int x = 0;\n"
+   "}");
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}");
+
+  FormatStyle CtorInitializerStyle = getLLVMStyle();
+  CtorInitializerStyle.SpaceBeforeCtorInitializerColon = false;
+  verifyFormat("class Foo : public Bar {};", CtorInitializerStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", CtorInitializerStyle);
+  verifyFormat("for (auto a : b) {\n}", CtorInitializerStyle);
+  verifyFormat("int x = a ? b : c;", CtorInitializerStyle);
+  verifyFormat("{\n"
+   "label1:\n"
+   "  int x = 0;\n"
+   "}",
+   CtorInitializerStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   CtorInitializerStyle);
+
+  FormatStyle InheritanceStyle = getLLVMStyle();
+  InheritanceStyle.SpaceBeforeInheritanceColon = false;
+  verifyFormat("class Foo: public Bar {};", InheritanceStyle);
+  verifyFormat("Foo::Foo() : foo(1) {}", InheritanceStyle);
+  verifyFormat("for (auto a : b) {\n}", InheritanceStyle);
+  verifyFormat("int x = a ? b : c;", InheritanceStyle);
+  verifyFormat("{\n"
+   "label2:\n"
+   "  int x = 0;\n"
+   "}",
+   InheritanceStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   InheritanceStyle);
+
+  FormatStyle ForLoopStyle = getLLVMStyle();
+  ForLoopStyle.SpaceBeforeRangeBasedForLoopColon = false;
+  verifyFormat("class Foo : public Bar {};", ForLoopStyle);
+  verifyFormat("Foo::Foo() : foo(1) {}", ForLoopStyle);
+  verifyFormat("for (auto a: b) {\n}", ForLoopStyle);
+  verifyFormat("int x = a ? b : c;", ForLoopStyle);
+  verifyFormat("{\n"
+   "label2:\n"
+   "  int x = 0;\n"
+   "}",
+   ForLoopStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   ForLoopStyle);
+
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  NoSpaceStyle.SpaceBeforeCtorInitializerColon = false;
+  NoSpaceStyle.SpaceBeforeInheritanceColon = false;
+  NoSpaceStyle.SpaceBeforeRangeBasedForLoopColon = false;
+  verifyFormat("class Foo: public Bar {};", NoSpaceStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", NoSpaceStyle);
+  verifyFormat("for (auto a: b) {\n}", NoSpaceStyle);
+  verifyFormat("int x = a ? b : c;", NoSpaceStyle);
+  verifyFormat("{\n"
+   "label3:\n"
+   "  int x = 0;\n"
+   "}",
+   NoSpaceStyle);
+  verifyFormat("switch (x) {\n"
+   "case 1:\n"
+   "default:\n"
+   "}",
+   NoSpaceStyle);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignments) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveAssignments = false;
@@ -8703,6 +8788,9 @@
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
+  CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
+  CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
+  CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2340,8 +2340,15 @@
 return true;
   if (Right.is(tok::comma))
 return false;
-  if (Right.isOneOf(TT_CtorInitializerColon, TT_ObjCBlockLParen))
+  if (Right.is(TT_ObjCBlockLParen))
 return true;
+  if (Right.is(TT_CtorInitializerColon))
+return Style.SpaceBeforeCtorInitializerColon;
+  if (Right.is(TT_InheritanceColon) && !Style.SpaceBeforeInheritanceColon)
+return false;
+  if 

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132838.
Typz added a comment.

rebase


https://reviews.llvm.org/D37813

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+Style));
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"} /* end of TESTSUITE(A) */",

[PATCH] D33440: clang-format: better handle statement macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132837.
Typz added a comment.

Use StatementMacro detection to improve brace type detection heuristics (in 
UnwrappedLineParser::calculateBraceTypes).


https://reviews.llvm.org/D33440

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2420,6 +2420,45 @@
getLLVMStyleWithColumns(40)));
 
   verifyFormat("MACRO(>)");
+
+  // Some macros contain an implicit semicolon
+  FormatStyle Style = getLLVMStyle();
+  Style.StatementMacros.push_back("FOO");
+  verifyFormat("FOO(a) int b = 0;");
+  verifyFormat("FOO(a)\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO(a);\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO(argc, argv, \"4.0.2\")\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO()\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("void f() {\n"
+   "  FOO(a)\n"
+   "  return a;\n"
+   "}",
+   Style);
+  verifyFormat("FOO(a)\n"
+   "FOO(b)",
+   Style);
+  verifyFormat("int a = 0;\n"
+   "FOO(b)\n"
+   "int c = 0;",
+   Style);
+  verifyFormat("int a = 0;\n"
+   "int x = FOO(a)\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("void foo(int a) { FOO(a) }\n"
+   "uint32_t bar() {}",
+   Style);
 }
 
 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
@@ -10244,6 +10283,12 @@
   CHECK_PARSE("ForEachMacros: [BOOST_FOREACH, Q_FOREACH]", ForEachMacros,
   BoostAndQForeach);
 
+  Style.StatementMacros.clear();
+  CHECK_PARSE("StatementMacros: [QUNUSED]", StatementMacros,
+  std::vector{"QUNUSED"});
+  CHECK_PARSE("StatementMacros: [QUNUSED, QT_REQUIRE_VERSION]", StatementMacros,
+  std::vector({"QUNUSED", "QT_REQUIRE_VERSION"}));
+
   Style.IncludeCategories.clear();
   std::vector ExpectedCategories = {{"abc/.*", 2},
   {".*", 1}};
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -118,6 +118,7 @@
   void parseObjCInterfaceOrImplementation();
   void parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
+  void parseStatementMacro();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -435,6 +435,10 @@
   }
   LBraceStack.pop_back();
   break;
+case tok::identifier:
+  if (!Tok->is(TT_StatementMacro))
+  break;
+  LLVM_FALLTHROUGH;
 case tok::at:
 case tok::semi:
 case tok::kw_if:
@@ -1093,6 +1097,10 @@
 return;
   }
 }
+if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
+  parseStatementMacro();
+  return;
+}
 // In all other cases, parse the declaration.
 break;
   default:
@@ -1242,6 +1250,11 @@
 return;
   }
 
+  if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
+parseStatementMacro();
+return;
+  }
+
   // See if the following token should start a new unwrapped line.
   StringRef Text = FormatTok->TokenText;
   nextToken();
@@ -2191,6 +2204,16 @@
   }
 }
 
+void UnwrappedLineParser::parseStatementMacro()
+{
+  nextToken();
+  if (FormatTok->is(tok::l_paren))
+parseParens();
+  if (FormatTok->is(tok::semi))
+nextToken();
+  addUnwrappedLine();
+}
+
 LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const UnwrappedLine ,
  StringRef Prefix = "") {
   llvm::dbgs() << Prefix << "Line(" << Line.Level << ")"
Index: lib/Format/FormatTokenLexer.h
===
--- lib/Format/FormatTokenLexer.h
+++ lib/Format/FormatTokenLexer.h
@@ -22,6 +22,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/ADT/MapVector.h"
 
 #include 
 
@@ -97,7 +98,8 @@
   // Index (in 'Tokens') of the last token that starts a new line.
   unsigned FirstInLineIndex;
   SmallVector Tokens;
-  SmallVector ForEachMacros;
+
+  llvm::SmallMapVector 

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> I don't think cases where there is only a semicolon-less macro are 
> particularly frequent/important either. You probably could even add a 
> semicolon in those cases, right? How frequent are such cases in your 
> codebase? Either way, they aren't fixed by this patch, so they aren't a good 
> reason to move forward with it.

Adding a semicolon works for indentation and behavior, but leads to compiler 
warning.
So a proper fix would require to change the macro, which can easily be tricky 
with macros containings branches or loops...

It should not happen that often, but in reality it actually happens often 
enough that I found the bug while simply testing clang-format...

> I still believe that this patch adds complexity for very little gain. And I 
> am not even sure it is correct. 
> isFunctionDeclarationName/getFunctionParameterList is just yet another 
> heuristic that might go wrong. And it might go wrong in both ways. You might 
> still miss some cases and you might start incorrectly formatting stuff as 
> functions. Fundamentally clang-format just doesn't have enough info to do 
> this correctly.

As such, it does not add such complexity I think, but indeed it is not 
completely correct: it works only in the case where no line breaks are expected 
around the body (since it is still parsed as a single UnwrappedLine). So it is 
a slight improvement, but not a complete fix.

`isFunctionDeclarationName()` is an heuristic as well, but provides better 
result and does not break the existing one (since there are actually quite a 
few tests, and none get broken by this patch :-) )

I think "overall" clang-format has enough information for this case, it is just 
not available at the right time: Unwrapping is done first, then token and 
matching parenthesis are analysed and lines anotated, and these token 
anotations are needed to improve the unwrapping behavior...
I would really want to call `isFunctionDeclarationName()` from 
`UnwrappedLineParser::calculateBraceTypes()`, but parenthesis matching, nesting 
level and operators identification are not available yet.

> ".. which can easily be overlooked". If they are overlooked, nobody cares 
> about the space either, so no harm done ;).

We want to use a formatter to ensure the code is consistent: e.g. to ensure 
things that may be overlooked by a human are actually formatted according to 
the rules.
We want people to trust the tool to do the right thing, so it is best to avoid 
as many errors as possible...


Repository:
  rC Clang

https://reviews.llvm.org/D42729



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> You might doubt it, but having written the code I can tell you that it's the 
> case.

Ok, you win :-)

> I see the argument why this indentation is not necessary in exactly the case 
> where the last parameter is multi-line and not wrapped to a new line itself: 
> You always have some indentation anyway because of the preceding parameter on 
> the same line.
>  However, for me the consistency is more important here, i.e. achieving that 
> we don't have a relative indentation change between:
>  [...] 
>  This formatting can easily alter between these two when line length vary 
> slightly and I think being able to pattern match that easily.

Not sure what you mean: a diff would not be trivial in either case...

> Yes, that means it is not consistent with:
> 
>   foo(bb +
>   c);
>
> 
> But there is actually a substantial difference in structure and so, I think 
> it is reasonable to not be consistent there.

It's reasonable from the perspective of the tool [i.e. to make the code of 
clang-format consistent], but IMHO not so consistent from the perspective of a 
user:
I (and most people I believe) would not manually add this seemingly unnecessary 
indentation, so I would prefer the tool to do the same; and fortunately it 
seems pretty trivial to implement.

Is there a way this can go in anyway, for exemple with a setting?


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I doubt this particular was intentional, esp. since this case never happens in 
the tests. I think it is more a side-effect of the (general) indent in "fake" 
parenthesis.
Here is an exemple:

Before this change:

  foo(a, bb +
 c);
  foo(b +
  c);
  foo(a,
  b +
  c,
  d);
  foo(b +
  c,
  d);

After this change:

  foo(a, bb +
 c);
  foo(b +
  c);
  foo(a,
  b +
  c,
  d);
  foo(b +
  c,
  d);

i.e. this patch only affect the 'first' scenario (e.g. wrapping expression in 
last argument) consistent with the second one (e.g. wrapping expression in 
first and only argument)


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

There should be no extra indent when wrapping only the expression used
as last argument. This is consistent with the behavior when the first
(and only) argument's expression is wrapped.

  foo(a, bb +
 c);
  foo(b +
  c);

This does not affect all other cases, where the argument itself is
wrapped:

  foo(a,
  b +
  c,
  d);
  foo(b +
  c,
  d);


Repository:
  rC Clang

https://reviews.llvm.org/D42787

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3998,6 +3998,21 @@
   verifyFormat("a(aa +\n"
"  aa,\n"
"  aa);");
+  verifyFormat(
+  "a(aa +\n"
+  "  aa) {}");
+  verifyFormat(
+  "a(aa +\n"
+  "  aa,\n"
+  "  b) {}");
+  verifyFormat(
+  "a(b, aaa +\n"
+  " aaa) {}");
+  verifyFormat(
+  "a(b,\n"
+  "  aaa +\n"
+  "  aaa,\n"
+  "  c) {}");
 
   // Indent consistently independent of call expression and unary operator.
   verifyFormat("aaa(bbb(\n"
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1094,6 +1094,7 @@
   bool SkipFirstExtraIndent =
   (Previous && (Previous->opensScope() ||
 Previous->isOneOf(tok::semi, tok::kw_return) ||
+(Previous->is(tok::comma) && !Newline) ||
 (Previous->getPrecedence() == prec::Assignment &&
  Style.AlignOperands) ||
 Previous->is(TT_ObjCMethodExpr)));


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3998,6 +3998,21 @@
   verifyFormat("a(aa +\n"
"  aa,\n"
"  aa);");
+  verifyFormat(
+  "a(aa +\n"
+  "  aa) {}");
+  verifyFormat(
+  "a(aa +\n"
+  "  aa,\n"
+  "  b) {}");
+  verifyFormat(
+  "a(b, aaa +\n"
+  " aaa) {}");
+  verifyFormat(
+  "a(b,\n"
+  "  aaa +\n"
+  "  aaa,\n"
+  "  c) {}");
 
   // Indent consistently independent of call expression and unary operator.
   verifyFormat("aaa(bbb(\n"
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1094,6 +1094,7 @@
   bool SkipFirstExtraIndent =
   (Previous && (Previous->opensScope() ||
 Previous->isOneOf(tok::semi, tok::kw_return) ||
+(Previous->is(tok::comma) && !Newline) ||
 (Previous->getPrecedence() == prec::Assignment &&
  Style.AlignOperands) ||
 Previous->is(TT_ObjCMethodExpr)));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132341.
Typz added a comment.

If the template declaration spans multiple lines, force wrap before the 
function/class declaration


Repository:
  rC Clang

https://reviews.llvm.org/D42684

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5367,7 +5367,7 @@
"const typename  aaa);");
 
   FormatStyle AlwaysBreak = getLLVMStyle();
-  AlwaysBreak.AlwaysBreakTemplateDeclarations = true;
+  AlwaysBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
   verifyFormat("template \nclass C {};", AlwaysBreak);
   verifyFormat("template \nvoid f();", AlwaysBreak);
   verifyFormat("template \nvoid f() {}", AlwaysBreak);
@@ -5385,6 +5385,32 @@
"public:\n"
"  E *f();\n"
"};");
+
+  FormatStyle NeverBreak = getLLVMStyle();
+  NeverBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_None;
+  verifyFormat("template  class C {};", NeverBreak);
+  verifyFormat("template  void f();", NeverBreak);
+  verifyFormat("template  void f() {}", NeverBreak);
+  verifyFormat("template \nvoid foo(aa ) {}",
+   NeverBreak);
+  verifyFormat("void aaa(\n"
+   "ccc);",
+   NeverBreak);
+  verifyFormat("template  class Fooo,\n"
+   "  template  class Baaar>\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  // T can be A, B or C.\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  class A {\n"
+   "public:\n"
+   "  E *f();\n"
+   "};", NeverBreak);
+  NeverBreak.PenaltyBreakTemplateDeclaration = 100;
+  verifyFormat("template  void\nfoo(aa ) {}",
+   NeverBreak);
 }
 
 TEST_F(FormatTest, WrapsTemplateParameters) {
@@ -10166,7 +10192,6 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
-  CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
@@ -10229,6 +10254,8 @@
   PenaltyBreakAssignment, 1234u);
   CHECK_PARSE("PenaltyBreakBeforeFirstCallParameter: 1234",
   PenaltyBreakBeforeFirstCallParameter, 1234u);
+  CHECK_PARSE("PenaltyBreakTemplateDeclaration: 1234",
+  PenaltyBreakTemplateDeclaration, 1234u);
   CHECK_PARSE("PenaltyExcessCharacter: 1234", PenaltyExcessCharacter, 1234u);
   CHECK_PARSE("PenaltyReturnTypeOnItsOwnLine: 1234",
   PenaltyReturnTypeOnItsOwnLine, 1234u);
@@ -10383,6 +10410,18 @@
   AlwaysBreakAfterReturnType,
   FormatStyle::RTBS_TopLevelDefinitions);
 
+  Style.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: None", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_None);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: BeforeFunction", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: All", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: false", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: true", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
   AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2171,6 +2171,8 @@
   return 2;
 return 1;
   }
+  if (Left.ClosesTemplateDeclaration)
+  return Style.PenaltyBreakTemplateDeclaration;
   if (Left.is(TT_ConditionalExpr))
 return prec::Conditional;
   prec::Level Level = Left.getPrecedence();
@@ -2641,7 +2643,7 @@
   if (Right.Previous->ClosesTemplateDeclaration &&
   Right.Previous->MatchingParen &&
   Right.Previous->MatchingParen->NestingLevel == 0 &&
-  

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132198.
Typz added a comment.

Force wrap after multi-line template declaration


Repository:
  rC Clang

https://reviews.llvm.org/D42684

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5367,7 +5367,7 @@
"const typename  aaa);");
 
   FormatStyle AlwaysBreak = getLLVMStyle();
-  AlwaysBreak.AlwaysBreakTemplateDeclarations = true;
+  AlwaysBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
   verifyFormat("template \nclass C {};", AlwaysBreak);
   verifyFormat("template \nvoid f();", AlwaysBreak);
   verifyFormat("template \nvoid f() {}", AlwaysBreak);
@@ -5385,6 +5385,32 @@
"public:\n"
"  E *f();\n"
"};");
+
+  FormatStyle NeverBreak = getLLVMStyle();
+  NeverBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_None;
+  verifyFormat("template  class C {};", NeverBreak);
+  verifyFormat("template  void f();", NeverBreak);
+  verifyFormat("template  void f() {}", NeverBreak);
+  verifyFormat("template \nvoid foo(aa ) {}",
+   NeverBreak);
+  verifyFormat("void aaa(\n"
+   "ccc);",
+   NeverBreak);
+  verifyFormat("template  class Fooo,\n"
+   "  template  class Baaar>\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  // T can be A, B or C.\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  class A {\n"
+   "public:\n"
+   "  E *f();\n"
+   "};", NeverBreak);
+  NeverBreak.PenaltyBreakTemplateDeclaration = 100;
+  verifyFormat("template  void\nfoo(aa ) {}",
+   NeverBreak);
 }
 
 TEST_F(FormatTest, WrapsTemplateParameters) {
@@ -10166,7 +10192,6 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
-  CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
@@ -10229,6 +10254,8 @@
   PenaltyBreakAssignment, 1234u);
   CHECK_PARSE("PenaltyBreakBeforeFirstCallParameter: 1234",
   PenaltyBreakBeforeFirstCallParameter, 1234u);
+  CHECK_PARSE("PenaltyBreakTemplateDeclaration: 1234",
+  PenaltyBreakTemplateDeclaration, 1234u);
   CHECK_PARSE("PenaltyExcessCharacter: 1234", PenaltyExcessCharacter, 1234u);
   CHECK_PARSE("PenaltyReturnTypeOnItsOwnLine: 1234",
   PenaltyReturnTypeOnItsOwnLine, 1234u);
@@ -10383,6 +10410,18 @@
   AlwaysBreakAfterReturnType,
   FormatStyle::RTBS_TopLevelDefinitions);
 
+  Style.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: None", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_None);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: BeforeFunction", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: All", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: false", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: true", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
   AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2171,6 +2171,8 @@
   return 2;
 return 1;
   }
+  if (Left.ClosesTemplateDeclaration)
+  return Style.PenaltyBreakTemplateDeclaration;
   if (Left.is(TT_ConditionalExpr))
 return prec::Conditional;
   prec::Level Level = Left.getPrecedence();
@@ -2641,7 +2643,7 @@
   if (Right.Previous->ClosesTemplateDeclaration &&
   Right.Previous->MatchingParen &&
   Right.Previous->MatchingParen->NestingLevel == 0 &&
-  Style.AlwaysBreakTemplateDeclarations)
+  Style.AlwaysBreakTemplateDeclarations 

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D42729#993159, @djasper wrote:

> I think this case is not important enough to fix. Please tell users to just 
> delete the useless semicolon.


I would agree if were simple to spot: but often this may manifest itself only 
with a missing space between the function parameters and the function body, 
which can easily be overlooked...
Another option may be to create new pass which "removes" that extra semicolon: 
this way we would both fix it and get things right on next pass.

However, the issue with a function which contains only a macro and which is 
followed by another function which returns an custom type cannot so easily be 
fixed:

  void abort() {
FOO()
  }
  uint32_t bar() {}

(note that this case works fine if the body of the function contains a 
semicolon or reserved keyword, or if the next function returns a base type 
[int, void...])


Repository:
  rC Clang

https://reviews.llvm.org/D42729



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

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

There are actually other cases, e.g. with macros "containing" the semicolon:

  void abort() {
FOO()
BAR()
  };

gets reformatted to this (still wrong with this patch, but the space after the 
parenthesis is added):

  void abort(){ FOO() BAR() };

And also this one (though it may be slightly different, there is no semicolon 
here):

  void abort() {
FOO()
  }
  uint32_t bar() {}

gets reformatted to:

  void abort(){ FOO() BAR() } uint32_t bar() {}

IMO the "real" fix would be to somehow let 
`TokenAnnotator::calculateFormattingInformation` split the current 
UnwrappedLine/AnnotatedLine, so that the 'end of the line' could be unwrapped 
properly: this would also allow keeping the invariant, and just implement some 
kind of backtracking. But it may be tricky to get the state right; maybe adding 
some child lines instead of just adding the lines may be simpler?


Repository:
  rC Clang

https://reviews.llvm.org/D42729



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

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:1911
+  if (Next && Next->is(tok::l_brace) && Next->BlockKind == BK_BracedInit)
+  Next->BlockKind = BK_Block;
+}

this may actually not be enough in all cases: to completely match the 
'standard' behavior we should be able to "split" the UnwrappedLine at both 
opening and ending brace, in this case.


Repository:
  rC Clang

https://reviews.llvm.org/D42729



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

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132130.
Typz added a comment.

update commit message


Repository:
  rC Clang

https://reviews.llvm.org/D42729

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11386,6 +11386,15 @@
"};");
 }
 
+TEST_F(FormatTest, MunchSemicolonAfterFunction) {
+  verifyFormat("void foo() { int i; };");
+  verifyFormat("void foo() {\n"
+   "  int i;\n"
+   "  int j;\n"
+   "};");
+  verifyFormat("void foo() {};");
+}
+
 TEST_F(FormatTest, ConfigurableContinuationIndentWidth) {
   FormatStyle TwoIndent = getLLVMStyleWithColumns(15);
   TwoIndent.ContinuationIndentWidth = 2;
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1783,9 +1783,12 @@
 }
 
 // This function heuristically determines whether 'Current' starts the name of a
-// function declaration.
-static bool isFunctionDeclarationName(const FormatToken ,
-  const AnnotatedLine ) {
+// function declaration, and returns the first token of parameters list in that
+// case.
+// @return The token which opens the parameters list (e.g. the opening
+// parenthesis), or nullptr if this is not a function declaration.
+static const FormatToken *getFunctionParameterList(const FormatToken ,
+   const AnnotatedLine ) {
   auto skipOperatorName = [](const FormatToken *Next) -> const FormatToken * {
 for (; Next; Next = Next->Next) {
   if (Next->is(TT_OverloadedOperatorLParen))
@@ -1809,58 +1812,58 @@
   const FormatToken *Next = Current.Next;
   if (Current.is(tok::kw_operator)) {
 if (Current.Previous && Current.Previous->is(tok::coloncolon))
-  return false;
+  return nullptr;
 Next = skipOperatorName(Next);
   } else {
 if (!Current.is(TT_StartOfName) || Current.NestingLevel != 0)
-  return false;
+  return nullptr;
 for (; Next; Next = Next->Next) {
   if (Next->is(TT_TemplateOpener)) {
 Next = Next->MatchingParen;
   } else if (Next->is(tok::coloncolon)) {
 Next = Next->Next;
 if (!Next)
-  return false;
+  return nullptr;
 if (Next->is(tok::kw_operator)) {
   Next = skipOperatorName(Next->Next);
   break;
 }
 if (!Next->is(tok::identifier))
-  return false;
+  return nullptr;
   } else if (Next->is(tok::l_paren)) {
 break;
   } else {
-return false;
+return nullptr;
   }
 }
   }
 
   // Check whether parameter list can belong to a function declaration.
   if (!Next || !Next->is(tok::l_paren) || !Next->MatchingParen)
-return false;
+return nullptr;
   // If the lines ends with "{", this is likely an function definition.
   if (Line.Last->is(tok::l_brace))
-return true;
+return Next;
   if (Next->Next == Next->MatchingParen)
-return true; // Empty parentheses.
+return Next; // Empty parentheses.
   // If there is an &/&& after the r_paren, this is likely a function.
   if (Next->MatchingParen->Next &&
   Next->MatchingParen->Next->is(TT_PointerOrReference))
-return true;
+return Next;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {
 if (Tok->is(tok::l_paren) && Tok->MatchingParen) {
   Tok = Tok->MatchingParen;
   continue;
 }
 if (Tok->is(tok::kw_const) || Tok->isSimpleTypeSpecifier() ||
 Tok->isOneOf(TT_PointerOrReference, TT_StartOfName, tok::ellipsis))
-  return true;
+  return Next;
 if (Tok->isOneOf(tok::l_brace, tok::string_literal, TT_ObjCMethodExpr) ||
 Tok->Tok.isLiteral())
-  return false;
+  return nullptr;
   }
-  return false;
+  return nullptr;
 }
 
 bool TokenAnnotator::mustBreakForReturnType(const AnnotatedLine ) const {
@@ -1899,8 +1902,14 @@
   FormatToken *Current = Line.First->Next;
   bool InFunctionDecl = Line.MightBeFunctionDecl;
   while (Current) {
-if (isFunctionDeclarationName(*Current, Line))
+if (const FormatToken *Tok = getFunctionParameterList(*Current, Line)) {
   Current->Type = TT_FunctionDeclarationName;
+
+  // Fix block kind, if it was mistakenly identified as a BracedInit
+  FormatToken *Next = Tok->MatchingParen->Next;
+  if (Next && Next->is(tok::l_brace) && Next->BlockKind == BK_BracedInit)
+  Next->BlockKind = BK_Block;
+}
 if (Current->is(TT_LineComment)) {
   if (Current->Previous->BlockKind == BK_BracedInit &&
   Current->Previous->opensScope())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D42729: clang-format: Fix formatting of function blocks followed by semicolon

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

In some case, the heuristic used to identify the type of blocks (in
UnwrappedLineParser) mistakens a Block for a BracedInit, when the
TokenAnnotator eventually finds (correclty) that this is a function
declaration.

This happens with empty function blocks followed by a semicolon.

This causes for exemple the following code to be mistakenly formatted:

  void abort(){};

instead of:

  void abort() {};


Repository:
  rC Clang

https://reviews.llvm.org/D42729

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11386,6 +11386,15 @@
"};");
 }
 
+TEST_F(FormatTest, MunchSemicolonAfterFunction) {
+  verifyFormat("void foo() { int i; };");
+  verifyFormat("void foo() {\n"
+   "  int i;\n"
+   "  int j;\n"
+   "};");
+  verifyFormat("void foo() {};");
+}
+
 TEST_F(FormatTest, ConfigurableContinuationIndentWidth) {
   FormatStyle TwoIndent = getLLVMStyleWithColumns(15);
   TwoIndent.ContinuationIndentWidth = 2;
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1783,9 +1783,12 @@
 }
 
 // This function heuristically determines whether 'Current' starts the name of a
-// function declaration.
-static bool isFunctionDeclarationName(const FormatToken ,
-  const AnnotatedLine ) {
+// function declaration, and returns the first token of parameters list in that
+// case.
+// @return The token which opens the parameters list (e.g. the opening
+// parenthesis), or nullptr if this is not a function declaration.
+static const FormatToken *getFunctionParameterList(const FormatToken ,
+   const AnnotatedLine ) {
   auto skipOperatorName = [](const FormatToken *Next) -> const FormatToken * {
 for (; Next; Next = Next->Next) {
   if (Next->is(TT_OverloadedOperatorLParen))
@@ -1809,58 +1812,58 @@
   const FormatToken *Next = Current.Next;
   if (Current.is(tok::kw_operator)) {
 if (Current.Previous && Current.Previous->is(tok::coloncolon))
-  return false;
+  return nullptr;
 Next = skipOperatorName(Next);
   } else {
 if (!Current.is(TT_StartOfName) || Current.NestingLevel != 0)
-  return false;
+  return nullptr;
 for (; Next; Next = Next->Next) {
   if (Next->is(TT_TemplateOpener)) {
 Next = Next->MatchingParen;
   } else if (Next->is(tok::coloncolon)) {
 Next = Next->Next;
 if (!Next)
-  return false;
+  return nullptr;
 if (Next->is(tok::kw_operator)) {
   Next = skipOperatorName(Next->Next);
   break;
 }
 if (!Next->is(tok::identifier))
-  return false;
+  return nullptr;
   } else if (Next->is(tok::l_paren)) {
 break;
   } else {
-return false;
+return nullptr;
   }
 }
   }
 
   // Check whether parameter list can belong to a function declaration.
   if (!Next || !Next->is(tok::l_paren) || !Next->MatchingParen)
-return false;
+return nullptr;
   // If the lines ends with "{", this is likely an function definition.
   if (Line.Last->is(tok::l_brace))
-return true;
+return Next;
   if (Next->Next == Next->MatchingParen)
-return true; // Empty parentheses.
+return Next; // Empty parentheses.
   // If there is an &/&& after the r_paren, this is likely a function.
   if (Next->MatchingParen->Next &&
   Next->MatchingParen->Next->is(TT_PointerOrReference))
-return true;
+return Next;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {
 if (Tok->is(tok::l_paren) && Tok->MatchingParen) {
   Tok = Tok->MatchingParen;
   continue;
 }
 if (Tok->is(tok::kw_const) || Tok->isSimpleTypeSpecifier() ||
 Tok->isOneOf(TT_PointerOrReference, TT_StartOfName, tok::ellipsis))
-  return true;
+  return Next;
 if (Tok->isOneOf(tok::l_brace, tok::string_literal, TT_ObjCMethodExpr) ||
 Tok->Tok.isLiteral())
-  return false;
+  return nullptr;
   }
-  return false;
+  return nullptr;
 }
 
 bool TokenAnnotator::mustBreakForReturnType(const AnnotatedLine ) const {
@@ -1899,8 +1902,14 @@
   FormatToken *Current = Line.First->Next;
   bool InFunctionDecl = Line.MightBeFunctionDecl;
   while (Current) {
-if (isFunctionDeclarationName(*Current, Line))
+if (const FormatToken *Tok = getFunctionParameterList(*Current, Line)) {
   Current->Type = TT_FunctionDeclarationName;
+
+  // Fix block kind, if it was mistakenly identified as a BracedInit
+  FormatToken *Next = 

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 131946.
Typz added a comment.

fix commit message


Repository:
  rC Clang

https://reviews.llvm.org/D42684

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5367,7 +5367,7 @@
"const typename  aaa);");
 
   FormatStyle AlwaysBreak = getLLVMStyle();
-  AlwaysBreak.AlwaysBreakTemplateDeclarations = true;
+  AlwaysBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
   verifyFormat("template \nclass C {};", AlwaysBreak);
   verifyFormat("template \nvoid f();", AlwaysBreak);
   verifyFormat("template \nvoid f() {}", AlwaysBreak);
@@ -5385,6 +5385,31 @@
"public:\n"
"  E *f();\n"
"};");
+
+  FormatStyle NeverBreak = getLLVMStyle();
+  NeverBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_None;
+  verifyFormat("template  class C {};", NeverBreak);
+  verifyFormat("template  void f();", NeverBreak);
+  verifyFormat("template  void f() {}", NeverBreak);
+  verifyFormat("template \nvoid foo(aa ) {}",
+   NeverBreak);
+  verifyFormat("void aaa(\n"
+   "ccc);",
+   NeverBreak);
+  verifyFormat("template  class Fooo,\n"
+   "  template  class Baaar> struct C {};",
+   NeverBreak);
+  verifyFormat("template  // T can be A, B or C.\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  class A {\n"
+   "public:\n"
+   "  E *f();\n"
+   "};", NeverBreak);
+  NeverBreak.PenaltyBreakTemplateDeclaration = 100;
+  verifyFormat("template  void\nfoo(aa ) {}",
+   NeverBreak);
 }
 
 TEST_F(FormatTest, WrapsTemplateParameters) {
@@ -10166,7 +10191,6 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
-  CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
@@ -10229,6 +10253,8 @@
   PenaltyBreakAssignment, 1234u);
   CHECK_PARSE("PenaltyBreakBeforeFirstCallParameter: 1234",
   PenaltyBreakBeforeFirstCallParameter, 1234u);
+  CHECK_PARSE("PenaltyBreakTemplateDeclaration: 1234",
+  PenaltyBreakTemplateDeclaration, 1234u);
   CHECK_PARSE("PenaltyExcessCharacter: 1234", PenaltyExcessCharacter, 1234u);
   CHECK_PARSE("PenaltyReturnTypeOnItsOwnLine: 1234",
   PenaltyReturnTypeOnItsOwnLine, 1234u);
@@ -10383,6 +10409,18 @@
   AlwaysBreakAfterReturnType,
   FormatStyle::RTBS_TopLevelDefinitions);
 
+  Style.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: None", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_None);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: BeforeFunction", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: All", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: false", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: true", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
   AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2171,6 +2171,8 @@
   return 2;
 return 1;
   }
+  if (Left.ClosesTemplateDeclaration)
+  return Style.PenaltyBreakTemplateDeclaration;
   if (Left.is(TT_ConditionalExpr))
 return prec::Conditional;
   prec::Level Level = Left.getPrecedence();
@@ -2641,7 +2643,7 @@
   if (Right.Previous->ClosesTemplateDeclaration &&
   Right.Previous->MatchingParen &&
   Right.Previous->MatchingParen->NestingLevel == 0 &&
-  Style.AlwaysBreakTemplateDeclarations)
+  Style.AlwaysBreakTemplateDeclarations == FormatStyle::BTDS_All)
 return true;
   if 

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

Introduce ``PenaltyBreakTemplateDeclaration`` to control the penalty,
and change ``AlwaysBreakTemplateDeclarations`` to an enum with 3 modes:

- ``None`` for automatic (e.g. penalty based) wrapping of template

declaration

- ``BeforeFunction`` for always wrapping before functions, and applying

penalty before classes (e.g. same as legacy behavior when
AlwaysBreakTemplateDeclarations=false)

- ``Always`` for always wrapping (e.g. same as legacy behavior when

AlwaysBreakTemplateDeclarations=true)


Repository:
  rC Clang

https://reviews.llvm.org/D42684

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5367,7 +5367,7 @@
"const typename  aaa);");
 
   FormatStyle AlwaysBreak = getLLVMStyle();
-  AlwaysBreak.AlwaysBreakTemplateDeclarations = true;
+  AlwaysBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
   verifyFormat("template \nclass C {};", AlwaysBreak);
   verifyFormat("template \nvoid f();", AlwaysBreak);
   verifyFormat("template \nvoid f() {}", AlwaysBreak);
@@ -5385,6 +5385,31 @@
"public:\n"
"  E *f();\n"
"};");
+
+  FormatStyle NeverBreak = getLLVMStyle();
+  NeverBreak.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_None;
+  verifyFormat("template  class C {};", NeverBreak);
+  verifyFormat("template  void f();", NeverBreak);
+  verifyFormat("template  void f() {}", NeverBreak);
+  verifyFormat("template \nvoid foo(aa ) {}",
+   NeverBreak);
+  verifyFormat("void aaa(\n"
+   "ccc);",
+   NeverBreak);
+  verifyFormat("template  class Fooo,\n"
+   "  template  class Baaar> struct C {};",
+   NeverBreak);
+  verifyFormat("template  // T can be A, B or C.\n"
+   "struct C {};",
+   NeverBreak);
+  verifyFormat("template  class A {\n"
+   "public:\n"
+   "  E *f();\n"
+   "};", NeverBreak);
+  NeverBreak.PenaltyBreakTemplateDeclaration = 100;
+  verifyFormat("template  void\nfoo(aa ) {}",
+   NeverBreak);
 }
 
 TEST_F(FormatTest, WrapsTemplateParameters) {
@@ -10166,7 +10191,6 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
-  CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
@@ -10229,6 +10253,8 @@
   PenaltyBreakAssignment, 1234u);
   CHECK_PARSE("PenaltyBreakBeforeFirstCallParameter: 1234",
   PenaltyBreakBeforeFirstCallParameter, 1234u);
+  CHECK_PARSE("PenaltyBreakTemplateDeclaration: 1234",
+  PenaltyBreakTemplateDeclaration, 1234u);
   CHECK_PARSE("PenaltyExcessCharacter: 1234", PenaltyExcessCharacter, 1234u);
   CHECK_PARSE("PenaltyReturnTypeOnItsOwnLine: 1234",
   PenaltyReturnTypeOnItsOwnLine, 1234u);
@@ -10383,6 +10409,18 @@
   AlwaysBreakAfterReturnType,
   FormatStyle::RTBS_TopLevelDefinitions);
 
+  Style.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_All;
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: None", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_None);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: BeforeFunction", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: All", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: false", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_BeforeFunction);
+  CHECK_PARSE("AlwaysBreakTemplateDeclarations: true", AlwaysBreakTemplateDeclarations,
+  FormatStyle::BTDS_All);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
   AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2171,6 +2171,8 @@
   return 2;
 return 1;
   }
+  if 

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

OK.

So you mean a solution like the one discussed earlier would be the way to go?

> I mean that we can configure macros in the format style, like "define A(X) 
> class X {". I'm not 100% sure whether we would just try to use the 
> Preprocessor for this, or whether we'd want to only allow a small subset of 
> actual macros, but the general idea would be the same: The 
> UnwrappedLineParser would parse the macro at the expansion location A(X) into 
> an unwrapped line, and then parse the expansion into a child line, with the 
> tokens tha tare not in the argument of the call being marked as fixed (parent 
> child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 
'reversible' description to support the namespace case, to allow generating the 
end-of-namespace comment)

Is there any work on that side, any timeline when this may be supported ?


https://reviews.llvm.org/D37813



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

[PATCH] D33440: clang-format: better handle statement macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D33440



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Ok, that's probably where our different opinions come from - I would want a 
> macro to be formatted to reflect how it's implemented, because otherwise I'm 
> going to be surprised when I look at the implementation, and I consider 
> surprises to be something to avoid in programming in general, where possible.

As "author of the coding rules" (and thus writer of clang-format config), I 
would agree with you.
But I think the tool itself should not decide to enforce this : maybe there are 
cases where the macro is "conceptually" a namespace, but should be indented as 
a class ; in this case I think the tool should quite dumb, and simply do what 
was asked: if asked to format the macro as a namespace, it should do it, 
whatever the implementation is.

That said, on top of this "user choice" the tool could be smart, and 
automatically handle all macros based on their actual implementation: if the 
macro is actually implemented with a namespace (and there is no "explicit" 
definition of how it should be handled), then indeed the tool could format the 
whole block as a namespace, and so on... But unfortunately that would imply 
parsing numerous #include to get the definitions, and is definitely not the 
goal of this patch: this patch is really about letting the "user" decide how to 
handle the macro.

> That said, I'm also then a bit confused by your tests - they seem to 
> currently format a mixture of namespace and class formatting, and combine the 
> indentation of a class-scope with namespace end comments.

This should not be the case: the goal here is that the macro is handled exactly 
like a namespace.
The NamespaceEndCommentFixer tests indeed contain some indentation, but it is 
not significant and is there only because I copied existing test cases and 
followed the same "style": but these tests do not actually perform any 
indentation, they simply handle the "end comment".
Do you have any specific test which confuses you?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I don't think this is really relevant for this tool: if someone changes the 
implementation of the macro, then *they* must indeed if it should not be 
formatted like a namespace (and keep the clang-format configuration unchanged), 
or if it should now be formatted like a class (and thus make changes to 
clang-format configuration). Here we are not defining what the macro does, but 
how clang-format should indent it : in most case I don't think the indentation 
format should actually depend on the way it is implemented...


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

As far as "parsing" and formatting inside the block is concerned, this is 
indeed unrelated (and would totally work if all macros where specified with 
some preprocessor definitions).

But identifying the 'opening' token and generating the matching 'closing' 
comment are totally related; it would seem very strange to have an option to 
specify that TESTSUITE() macros are parsed as namespace, then another option to 
indicate that namespace declared by this macros are actually closed with 
another macro call...


https://reviews.llvm.org/D37813



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Indeed, looks good, thanks!

Though that exacerbates the alignment issue, I now get things like this:

  enum {
SomeLongerEnum, // comment
SomeThing,  // comment
Foo, // something
  } 
  ^ (column limit)

The comment on 'Foo' would overflow a bit, but it gets unindented during 
"alingment" stage, which kind of 'breaks' the decision that was made earlier on 
*not* to break the comment...


https://reviews.llvm.org/D33589



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Definitely that would be much more expressive. But this approach is also 
incomplete: in case of namespace (and possibly others?), we also need to 
perform the reverse operation, e.g. to "generate" a macro call for rewriting 
the closing comment.

On top of this, I think that is really not trivial, I would prefer to move 
forward with these "simpler" patch, and change the whole macro configurations 
part in later commits...


https://reviews.llvm.org/D37813



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I think the difference between code and comments is that code "words" are 
easily 10 characters or more, whereas actual words (in comments) are very often 
less than 10 characters: so code overflowing by 10 characters is not very 
frequent. whereas small words in comment will often get closer to the "extra" 
limit.

That said, I tried with your latest change ("Restructure how we break tokens", 
sha1:64d42a2fb85ece5987111ffb908c6bc7f7431dd4). and it's working about fine 
now. For the most part it seems to wrap when I would expect it, great work!
I have seen 2 "issues" though:

- Often I see that the last word before reflowing is not wrapped (eventhough it 
overlaps the line length); I did not count penalties so I cannot confirm this 
is really an issue or just a borderline scenario.
- Alignment seems better than before, but since there is no penalty for 
breaking alignment it will always try to unindent to compensate for overflowing 
characters...

Seeing this, I guess this patch does not make much sense anymore, I'll see if I 
make some improvements for these two issues, in separate patches.


https://reviews.llvm.org/D33589



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> @klimek wrote:
>  In the above example, we add 3 line breaks, and we'd add 1 (or more) 
> additional line breaks when reflowing below the column limit.
>  I agree that that can lead to different overall outcomes, but I don't see 
> how the approach of this patch really fixes it - it will only ever reflow 
> below the column limit, so it'll also lead to states for long lines where 
> reflowing and leaving chars over the line limit might be the overall best 
> choice (unless I'm missing something)?

Definitely, this patch may not find the 'optimal' solution. What I mean is that 
we reduce the `PenaltyExcessCharacter` value to allow "occasionally" breaking 
the limit: instead of a hard limit, we want to allow lines to sometimes break 
the limit, but definitely not *all* the time. Both patch work fine when the 
code is "correct", i.e. there is indeed only a few lines which break the limit.

But the local decision approach behaves really wrong IMHO when the code is 
formatted beyond the column: it can very easily reformat in such a way that the 
comment is reflown to what looks like a longer column limit. I currently have a 
ratio of 10 between  PenaltyBreakComment and PenaltyExcessCharacter (which 
empirically seemed to give a decent compromise, and match how our code is 
formatted; I need to try more with your patch, to see if we can get better 
values...): with this setting, a "non wrapped" comment will actually be reflown 
to ColumnLimit+10...

When we do indeed reflow, I think we may be stricter than this, to get 
something that really looks like it obeys the column limit. If this is 
'optimal' in the sense that we may have some overflow still, that is fine, but 
really not the primary requirement IMHO.


https://reviews.llvm.org/D33589



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Btw, another issue I am having is that reflowing does not respect the 
alignment. For exemple:

  enum {
 Foo,///< This is a very long comment
 Bar,///< This is shorter  
 BarBar, ///< This is shorter
  } Stuff;

will be reflown to :

  enum {
 Foo, ///< This is a very long
  ///< comment
 Bar, ///< This is shorter  
 BarBar, ///< This is shorter
  } Stuff;

when I would expect:

  enum {
 Foo,///< This is a very
 ///< long comment
 Bar,///< This is shorter  
 BarBar, ///< This is shorter
  } Stuff;

I see there is no penalty for breaking alignment, which may be accounted for 
when compressing the whitespace 'before' the comment... Or maybe simply the 
breaking should be smarter, to try to keep the alignment; but I am not sure it 
can be done without another kind of 'global' optimization of the comment 
reflow... Any idea/hint?


https://reviews.llvm.org/D33589



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D33589#925903, @klimek wrote:

> I think this patch doesn't handle a couple of cases that I'd like to see 
> handled. A counter-proposal with different trade-offs is in 
> https://reviews.llvm.org/D40068.


It may be simpler (though not to my eyes, I am not knowledgeable enough to 
really understand how you go this fixed...), and works fine for "almost 
correct" comments: e.g. when there are indeed just a few extra characters 
overall. But it still procudes strange result when each line of the (long) 
comment is too long, but not enough to trigger a line-wrap by itself.

Since that version has landed already, not sure how to improve on this. I could 
probably rewrite my patch on master, but it seems a bit redundant. As a simpler 
fix, I could imagine adding a "total" overflow counter, to allow detecting the 
situation; but when this is detected (e.g. on subsequent lines) we would need 
to "backtrack" and revisit the initial decision...


https://reviews.llvm.org/D33589



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D33589#924716, @klimek wrote:

> One interesting trade-off I'm running into:
>  My gut feeling is that we really want to make local decisions about whether 
> we want to break/reflow - this makes the code significantly simpler (IMO), 
> and handles all tests in this patch correctly, but is fundamentally limiting 
> the global optimizations we can do. Specifically, we would not correctly 
> reflow this:
>
>   //   |< limit
>   // foo bar
>   // baz
>   // x
>
> to
>
>   // foo
>   // bar
>   // baz x
>
> when the excess character limit is low.


As I can see with your patch, local decision does not account for accumulated 
penalty on multi-line comment, and will thus give unexpected (e.g. no change) 
result when each line overlaps by a few characters, but not enough to trigger a 
break at this line.

> That would be a point for global optimization, but I find it really hard to 
> understand exactly why it's ok to do it. Won't we get things like this wrong:
> 
>   Limit: 13
>   // foo  bar baz
>   // bab  bob
> 
> as we'll not compress whitespace?

Indeed, this patch would not trigger whitespace compression when not reflowing; 
it would compare "not doing anything" (no reflow, no whitespace compression) 
with the complete reflowing (including whitespace compression). I don't think 
that would break anything, but indeed we could possibly get even better result 
by trying to apply whitespace compression in the no-reflow case [which should 
be simple, just a bit more code at line 1376 in the version of the patch].


https://reviews.llvm.org/D33589



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

[PATCH] D40068: Implement more accurate penalty & trade-offs while breaking protruding tokens.

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Generally, this indeed improves the situation (though I cannot say much about 
the code itself, it is still too subtle for my shallow knowledge of 
clang-format).

But it seems to give some strange looking result with long comments: it seems 
like the decision is made at each line (e.g. is it better to wrap this line or 
overflow a bit), so we can get a comment where each line overflows by a few 
characters, even if the total is worse... For exemple, say we have a 100 lines 
of comment, with 9 characters overflow on each line, and an excess character 
penalty of 30 : with this patch nothing will be re-wrapped (9*30 = 270 is less 
the the 300 penalty for wrapping); but the total penatly would be 900

(btw, it seems this got merged, but the ticket does not reflect it)




Comment at: lib/Format/BreakableToken.cpp:293
   Split ReflowSplit =
-  FullWidth <= ColumnLimit
-  ? Split(TrimmedText.size(), Text.size() - TrimmedText.size())
-  : getCommentSplit(Text, ReflowStartColumn, ColumnLimit,
-Style.TabWidth, Encoding);
-
+//  FullWidth <= ColumnLimit
+  //? 

commented-out code to be removed?



Comment at: lib/Format/BreakableToken.cpp:300
+
+  return ReflowSplit;
   // We need to be extra careful here, because while it's OK to keep a long 
line

unconditional return --> remove the end of the function



Comment at: lib/Format/BreakableToken.cpp:572
+  return Split(0, 0);
+  /*
   StringRef TrimmedContent = Content[LineIndex].ltrim(Blanks);

remove commented-out code



Comment at: lib/Format/BreakableToken.cpp:603
+  // FIXME: pull together with getLineLegnthAfterSplit (same as for
+  // BreakableLineCommentSeciotn.
+  unsigned ContentStartColumn = 0;

BreakableLineCommentSection


https://reviews.llvm.org/D40068



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D37813



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

[PATCH] D33440: clang-format: better handle statement macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D33440



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

[PATCH] D33440: clang-format: better handle statement macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 18.
Typz marked 5 inline comments as done.
Typz added a comment.

Address review comments


https://reviews.llvm.org/D33440

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2420,6 +2420,42 @@
getLLVMStyleWithColumns(40)));
 
   verifyFormat("MACRO(>)");
+
+  // Some macros contain an implicit semicolon
+  FormatStyle Style = getLLVMStyle();
+  Style.StatementMacros.push_back("FOO");
+  verifyFormat("FOO(a) int b = 0;");
+  verifyFormat("FOO(a)\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO(a);\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO(argc, argv, \"4.0.2\")\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO()\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("FOO\n"
+   "int b = 0;",
+   Style);
+  verifyFormat("void f() {\n"
+   "  FOO(a)\n"
+   "  return a;\n"
+   "}",
+   Style);
+  verifyFormat("FOO(a)\n"
+   "FOO(b)",
+   Style);
+  verifyFormat("int a = 0;\n"
+   "FOO(b)\n"
+   "int c = 0;",
+   Style);
+  verifyFormat("int a = 0;\n"
+   "int x = FOO(a)\n"
+   "int b = 0;",
+   Style);
 }
 
 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
@@ -10244,6 +10280,12 @@
   CHECK_PARSE("ForEachMacros: [BOOST_FOREACH, Q_FOREACH]", ForEachMacros,
   BoostAndQForeach);
 
+  Style.StatementMacros.clear();
+  CHECK_PARSE("StatementMacros: [QUNUSED]", StatementMacros,
+  std::vector{"QUNUSED"});
+  CHECK_PARSE("StatementMacros: [QUNUSED, QT_REQUIRE_VERSION]", StatementMacros,
+  std::vector({"QUNUSED", "QT_REQUIRE_VERSION"}));
+
   Style.IncludeCategories.clear();
   std::vector ExpectedCategories = {{"abc/.*", 2},
   {".*", 1}};
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -118,6 +118,7 @@
   void parseObjCInterfaceOrImplementation();
   void parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
+  void parseStatementMacro();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   void tryToParseJSFunction();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1093,6 +1093,10 @@
 return;
   }
 }
+if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
+  parseStatementMacro();
+  return;
+}
 // In all other cases, parse the declaration.
 break;
   default:
@@ -1242,6 +1246,11 @@
 return;
   }
 
+  if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
+parseStatementMacro();
+return;
+  }
+
   // See if the following token should start a new unwrapped line.
   StringRef Text = FormatTok->TokenText;
   nextToken();
@@ -2191,6 +2200,16 @@
   }
 }
 
+void UnwrappedLineParser::parseStatementMacro()
+{
+  nextToken();
+  if (FormatTok->is(tok::l_paren))
+parseParens();
+  if (FormatTok->is(tok::semi))
+nextToken();
+  addUnwrappedLine();
+}
+
 LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const UnwrappedLine ,
  StringRef Prefix = "") {
   llvm::dbgs() << Prefix << "Line(" << Line.Level << ")"
Index: lib/Format/FormatTokenLexer.h
===
--- lib/Format/FormatTokenLexer.h
+++ lib/Format/FormatTokenLexer.h
@@ -22,6 +22,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/ADT/MapVector.h"
 
 #include 
 
@@ -97,7 +98,8 @@
   // Index (in 'Tokens') of the last token that starts a new line.
   unsigned FirstInLineIndex;
   SmallVector Tokens;
-  SmallVector ForEachMacros;
+
+  llvm::SmallMapVector Macros;
 
   bool FormattingDisabled;
 
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -37,8 +37,9 @@
   Lex->SetKeepWhitespaceMode(true);
 
   for (const std::string  : Style.ForEachMacros)
-ForEachMacros.push_back((ForEachMacro));
-  std::sort(ForEachMacros.begin(), 

[PATCH] D33440: clang-format: better handle statement macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D33440#920205, @djasper wrote:

> Out of curiosity, will this be able to fix the two situations that you get 
> for python extension?
>  There, you usually have a PyObject_HEAD with out semicolon in a struct and 
> than a PyObject_HEAD_INIT(..) in a braced init list. More info:
>  http://starship.python.net/crew/mwh/toext/node20.html


`PyObject_HEAD` is defined (by defaut) as:

  Py_ssize_t ob_refcnt;
  PyTypeObject *ob_type;

so declaring it as a statement macro should allow clang-format to process it 
correctly. (though this is a variable-like macro, so I need to check if this 
case is supported)

`PyObject_HEAD_INIT` ends with a comma, which is not supported yet. We would 
need to add another kind of macro to support that case.


https://reviews.llvm.org/D33440



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Unless I'm missing something, I'd agree with Daniel; this is not a rule 
> that's widely used, and I'd say reformatting a code base to the 
> clang-formatted variant will not regress readability.

Unfortunately coding rules are not just about readability, but also about 
consistency at entreprise scale, thus as a general rule we should not change 
the established rules in an organizatoin.
There is history in rules, we have code which already uses these rules, so for 
consistency we must stick to the old rules (even if we would not necessarily 
choose the same rules if we were to start from scratch).


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Sorry for the long response time. I don't see this style rule expressed 
> explicitly in the Skia or QtCreator style guides - is this something that 
> just happens to be done sometimes in the code bases?

This is present in code base. Those project's style guides are not precise 
enough, and do not document the behavior for this case.

> I have problems understanding the rules from the discussion (as well as the 
> code / test tbh), is there a really concise way to give me the core of the 
> idea?

The case I am trying to address is to really keep the _operands_ aligned after 
an assignment or `return`, in case line is wrapped *before* the operator.

  int a = b
+ c;
  return a
   + b;

while the current behavior with `Style.AlignOperands = true; 
BreakBeforeBinaryOperators = true` is to align the wrapped operator with the 
previous line's operand:

  int a = b
  + c;
  return a
 + b;

In the discussion, it appeared that this behavior is not a error (with respect 
to the name), but an expected behavior for most coding rules: hence the 
introduction of a third AlignOperands mode ("AlignAfterOperator"), to handle 
this new case.

From there the code actually got a bit more complex to support various corner 
cases (e.g. operators with different number of characters, wrapperd first line, 
do not unindent more than the enclosing brace...)


https://reviews.llvm.org/D32478



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

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D32478



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

[PATCH] D33440: clang-format: better handle statement macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D33440



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

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping ?


https://reviews.llvm.org/D33589



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