[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-02-06 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton marked an inline comment as done.
benhamilton added a comment.

> I don't understand why do we introduce an enum option if we are keeping the 
> default behavior for Google style. IMO we should have a single behavior for 
> any style and enforce it.

https://reviews.llvm.org/D42708 changes the default behavior for the Google 
style.

I'm fine with opening up a discussion to make this the default for all styles, 
but I think that should be a separate discussion.


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-02-06 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir added a comment.

I don't understand why do we introduce an enum option if we are keeping the 
default behavior for Google style. IMO we should have a single behavior for any 
style and enforce it.


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-02-02 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton marked 2 inline comments as done.
benhamilton added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:1214
 // FIXME: We likely want to do this for more combinations of brackets.
 // Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {

benhamilton wrote:
> stephanemoore wrote:
> > With the new parameter does this comment still apply? Maybe we can remove 
> > it?
> Good point.
> 
> This is about <> brackets nested inside () parens, which I believe is 
> applicable to Objective-C 2.0 generics passed to C/C++ functions, but not to 
> protocol conformance lists.
> 
> I'll send out a separate diff to add tests and clean up this comment, since I 
> think the behavior is wanted for ObjC as well, but I think it's unrelated to 
> this diff.
Followup in D42864.


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-02-02 Thread Ben Hamilton via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC324131: [clang-format] New format param 
ObjCBinPackProtocolList (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D42650?vs=132187=132650#toc

Repository:
  rC Clang

https://reviews.llvm.org/D42650

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

Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -390,6 +390,17 @@
   /// \endcode
   bool BinPackParameters;
 
+  /// \brief The style of wrapping parameters on the same line (bin-packed) or
+  /// on one line each.
+  enum BinPackStyle {
+/// Automatically determine parameter bin-packing behavior.
+BPS_Auto,
+/// Always bin-pack parameters.
+BPS_Always,
+/// Never bin-pack parameters.
+BPS_Never,
+  };
+
   /// \brief The style of breaking before or after binary operators.
   enum BinaryOperatorStyle {
 /// Break after operators.
@@ -1299,6 +1310,38 @@
   /// \brief The indentation used for namespaces.
   NamespaceIndentationKind NamespaceIndentation;
 
+  /// \brief Controls bin-packing Objective-C protocol conformance list
+  /// items into as few lines as possible when they go over ``ColumnLimit``.
+  ///
+  /// If ``Auto`` (the default), delegates to the value in
+  /// ``BinPackParameters``. If that is ``true``, bin-packs Objective-C
+  /// protocol conformance list items into as few lines as possible
+  /// whenever they go over ``ColumnLimit``.
+  ///
+  /// If ``Always``, always bin-packs Objective-C protocol conformance
+  /// list items into as few lines as possible whenever they go over
+  /// ``ColumnLimit``.
+  ///
+  /// If ``Never``, lays out Objective-C protocol conformance list items
+  /// onto individual lines whenever they go over ``ColumnLimit``.
+  ///
+  /// \code
+  ///Always (or Auto, if BinPackParameters=true):
+  ///@interface c () <
+  ///c, c,
+  ///c, c> {
+  ///}
+  ///
+  ///Never (or Auto, if BinPackParameters=false):
+  ///@interface d () <
+  ///d,
+  ///d,
+  ///d,
+  ///d> {
+  ///}
+  /// \endcode
+  BinPackStyle ObjCBinPackProtocolList;
+
   /// \brief The number of characters to use for indentation of ObjC blocks.
   /// \code{.objc}
   ///ObjCBlockIndentWidth: 4
@@ -1681,6 +1724,7 @@
MacroBlockEnd == R.MacroBlockEnd &&
MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
NamespaceIndentation == R.NamespaceIndentation &&
+   ObjCBinPackProtocolList == R.ObjCBinPackProtocolList &&
ObjCBlockIndentWidth == R.ObjCBlockIndentWidth &&
ObjCSpaceAfterProperty == R.ObjCSpaceAfterProperty &&
ObjCSpaceBeforeProtocolList == R.ObjCSpaceBeforeProtocolList &&
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -105,6 +105,14 @@
   }
 };
 
+template <> struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::BinPackStyle ) {
+IO.enumCase(Value, "Auto", FormatStyle::BPS_Auto);
+IO.enumCase(Value, "Always", FormatStyle::BPS_Always);
+IO.enumCase(Value, "Never", FormatStyle::BPS_Never);
+  }
+};
+
 template <> struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::BinaryOperatorStyle ) {
 IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -378,6 +386,7 @@
 IO.mapOptional("MacroBlockEnd", Style.MacroBlockEnd);
 IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
 IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
+IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
 IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
 IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty);
 IO.mapOptional("ObjCSpaceBeforeProtocolList",
@@ -637,6 +646,7 @@
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
+  LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1222,9 +1222,20 @@
 Current.MatchingParen->getPreviousNonComment() &&
 Current.MatchingParen->getPreviousNonComment()->is(tok::comma);
 
+//

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-02-02 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:1214
 // FIXME: We likely want to do this for more combinations of brackets.
 // Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {

stephanemoore wrote:
> With the new parameter does this comment still apply? Maybe we can remove it?
Good point.

This is about <> brackets nested inside () parens, which I believe is 
applicable to Objective-C 2.0 generics passed to C/C++ functions, but not to 
protocol conformance lists.

I'll send out a separate diff to add tests and clean up this comment, since I 
think the behavior is wanted for ObjC as well, but I think it's unrelated to 
this diff.


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-02-01 Thread Stephane Moore via Phabricator via cfe-commits 
stephanemoore added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:1214
 // FIXME: We likely want to do this for more combinations of brackets.
 // Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {

With the new parameter does this comment still apply? Maybe we can remove it?


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-01-31 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton added a comment.

Thanks! @jolesiak is not available for the rest of the week, so should I just 
submit this as-is, or wait until next week?


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-01-31 Thread Stephane Moore via Phabricator via cfe-commits 
stephanemoore accepted this revision.
stephanemoore added a comment.
This revision is now accepted and ready to land.

Looks good to me  Thanks for driving this!


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

2018-01-31 Thread Ben Hamilton via Phabricator via cfe-commits 
benhamilton updated this revision to Diff 132187.
benhamilton marked 2 inline comments as done.
benhamilton added a comment.

- BinPackObjCProtocolList -> ObjCBinPackProtocolList


Repository:
  rC Clang

https://reviews.llvm.org/D42650

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

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -281,15 +281,28 @@
"c, c,\n"
"c, c> {\n"
"}");
-
-  Style.BinPackParameters = false;
+  Style.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
   verifyFormat("@interface d () <\n"
"d,\n"
"d,\n"
"d,\n"
"d> {\n"
"}");
 
+  Style.BinPackParameters = false;
+  Style.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
+  verifyFormat("@interface e () <\n"
+   "e,\n"
+   "e,\n"
+   "e,\n"
+   "e> {\n"
+   "}");
+  Style.ObjCBinPackProtocolList = FormatStyle::BPS_Always;
+  verifyFormat("@interface f () <\n"
+   "f, f,\n"
+   "f, f> {\n"
+   "}");
+
   Style = getGoogleStyle(FormatStyle::LK_ObjC);
   verifyFormat("@interface Foo : NSObject  {\n"
" @public\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -105,6 +105,14 @@
   }
 };
 
+template <> struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::BinPackStyle ) {
+IO.enumCase(Value, "Auto", FormatStyle::BPS_Auto);
+IO.enumCase(Value, "Always", FormatStyle::BPS_Always);
+IO.enumCase(Value, "Never", FormatStyle::BPS_Never);
+  }
+};
+
 template <> struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::BinaryOperatorStyle ) {
 IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -378,6 +386,7 @@
 IO.mapOptional("MacroBlockEnd", Style.MacroBlockEnd);
 IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
 IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
+IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
 IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
 IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty);
 IO.mapOptional("ObjCSpaceBeforeProtocolList",
@@ -637,6 +646,7 @@
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
+  LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1222,9 +1222,20 @@
 Current.MatchingParen->getPreviousNonComment() &&
 Current.MatchingParen->getPreviousNonComment()->is(tok::comma);
 
+// If ObjCBinPackProtocolList is unspecified, fall back to BinPackParameters
+// for backwards compatibility.
+bool ObjCBinPackProtocolList =
+(Style.ObjCBinPackProtocolList == FormatStyle::BPS_Auto &&
+ Style.BinPackParameters) ||
+Style.ObjCBinPackProtocolList == FormatStyle::BPS_Always;
+
+bool BinPackDeclaration =
+(State.Line->Type != LT_ObjCDecl && Style.BinPackParameters) ||
+(State.Line->Type == LT_ObjCDecl && ObjCBinPackProtocolList);
+
 AvoidBinPacking =
 (Style.Language == FormatStyle::LK_JavaScript && EndsInComma) ||
-(State.Line->MustBeDeclaration && !Style.BinPackParameters) ||
+(State.Line->MustBeDeclaration && !BinPackDeclaration) ||
 (!State.Line->MustBeDeclaration && !Style.BinPackArguments) ||
 (Style.ExperimentalAutoDetectBinPacking &&
  (Current.PackingKind == PPK_OnePerLine ||
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -390,6 +390,17 @@
   /// \endcode
   bool BinPackParameters;
 
+  /// \brief The style of wrapping parameters on the same line (bin-packed) or
+  /// on one line each.
+  enum BinPackStyle {
+/// Automatically determine parameter bin-packing behavior.
+BPS_Auto,
+/// Always bin-pack