[PATCH] D42650: [clang-format] New format param BinPackObjCProtocolList
benhamilton marked 3 inline comments as done.
benhamilton added a comment.
Thanks for the review!
Comment at: lib/Format/Format.cpp:765
GoogleStyle.ColumnLimit = 100;
+GoogleStyle.BinPackObjCProtocolList = FormatStyle::BPS_Never;
}
stephanemoore wrote:
> I propose that we defer this to a future commit while we establish consensus
> at Google.
Moved to D42708.
Comment at: unittests/Format/FormatTestObjC.cpp:325-334
+ Style.ColumnLimit = 40;
+ // BinPackParameters should be true by default.
+ verifyFormat("void (int e, int e,\n"
+ " int e, int e);\n");
+ // BinPackObjCProtocolList should be BPS_Never by default.
+ verifyFormat("@interface f () <\n"
+ "f,\n"
stephanemoore wrote:
> Similar to my other comment, I propose deferring the changes to Google style
> to a future commit.
Moved to D42708.
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 BinPackObjCProtocolList
benhamilton updated this revision to Diff 132046.
benhamilton added a comment.
Move Google style changes out. Use clearer name for local variable.
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.BinPackObjCProtocolList = FormatStyle::BPS_Never;
verifyFormat("@interface d () <\n"
"d,\n"
"d,\n"
"d,\n"
"d> {\n"
"}");
+ Style.BinPackParameters = false;
+ Style.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
+ verifyFormat("@interface e () <\n"
+ "e,\n"
+ "e,\n"
+ "e,\n"
+ "e> {\n"
+ "}");
+ Style.BinPackObjCProtocolList = 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 &IO, FormatStyle::BinPackStyle &Value) {
+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 &IO, FormatStyle::BinaryOperatorStyle &Value) {
IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -323,6 +331,7 @@
Style.AlwaysBreakTemplateDeclarations);
IO.mapOptional("BinPackArguments", Style.BinPackArguments);
IO.mapOptional("BinPackParameters", Style.BinPackParameters);
+IO.mapOptional("BinPackObjCProtocolList", Style.BinPackObjCProtocolList);
IO.mapOptional("BraceWrapping", Style.BraceWrapping);
IO.mapOptional("BreakBeforeBinaryOperators",
Style.BreakBeforeBinaryOperators);
@@ -599,6 +608,7 @@
LLVMStyle.AlwaysBreakTemplateDeclarations = false;
LLVMStyle.BinPackArguments = true;
LLVMStyle.BinPackParameters = true;
+ LLVMStyle.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
LLVMStyle.BreakBeforeTernaryOperators = true;
LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
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 BinPackObjCProtocolList is unspecified, fall back to BinPackParameters
+// for backwards compatibility.
+bool BinPackObjCProtocolList =
+(Style.BinPackObjCProtocolList == FormatStyle::BPS_Auto &&
+ Style.BinPackParameters) ||
+Style.BinPackObjCProtocolList == FormatStyle::BPS_Always;
+
+bool BinPackDeclaration =
+(State.Line->Type != LT_ObjCDecl && Style.BinPackParameters) ||
+(State.Line->Type == LT_ObjCDecl && BinPackObjCProtocolList);
+
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,49 @@
/// \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
[PATCH] D42650: [clang-format] New format param BinPackObjCProtocolList
stephanemoore requested changes to this revision.
stephanemoore added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Format/ContinuationIndenter.cpp:1232
+
+bool BinPackParameters =
+(State.Line->Type != LT_ObjCDecl && Style.BinPackParameters) ||
Given that we now have a new setting distinguishing bin packing of Objective-C
protocol conformance lists and bin packing of parameters perhaps this local
variable should be named more generally? Maybe "BinPackDeclaration" would be
appropriate because this local variable takes effect if
`State.Line->MustBeDeclaration` is true?
Comment at: lib/Format/Format.cpp:765
GoogleStyle.ColumnLimit = 100;
+GoogleStyle.BinPackObjCProtocolList = FormatStyle::BPS_Never;
}
I propose that we defer this to a future commit while we establish consensus at
Google.
Comment at: unittests/Format/FormatTestObjC.cpp:325-334
+ Style.ColumnLimit = 40;
+ // BinPackParameters should be true by default.
+ verifyFormat("void (int e, int e,\n"
+ " int e, int e);\n");
+ // BinPackObjCProtocolList should be BPS_Never by default.
+ verifyFormat("@interface f () <\n"
+ "f,\n"
Similar to my other comment, I propose deferring the changes to Google style to
a future commit.
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 BinPackObjCProtocolList
benhamilton updated this revision to Diff 131841.
benhamilton added a comment.
Rebase
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.BinPackObjCProtocolList = FormatStyle::BPS_Never;
verifyFormat("@interface d () <\n"
"d,\n"
"d,\n"
"d,\n"
"d> {\n"
"}");
+ Style.BinPackParameters = false;
+ Style.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
+ verifyFormat("@interface e () <\n"
+ "e,\n"
+ "e,\n"
+ "e,\n"
+ "e> {\n"
+ "}");
+ Style.BinPackObjCProtocolList = 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"
@@ -309,13 +322,16 @@
verifyFormat("@interface Foo (HackStuff) \n"
"+ (id)init;\n"
"@end");
- Style.BinPackParameters = false;
- Style.ColumnLimit = 80;
- verifyFormat("@interface a () <\n"
- "a,\n"
- ",\n"
- "aa,\n"
- "> {\n"
+ Style.ColumnLimit = 40;
+ // BinPackParameters should be true by default.
+ verifyFormat("void (int e, int e,\n"
+ " int e, int e);\n");
+ // BinPackObjCProtocolList should be BPS_Never by default.
+ verifyFormat("@interface f () <\n"
+ "f,\n"
+ "f,\n"
+ "f,\n"
+ "f> {\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 &IO, FormatStyle::BinPackStyle &Value) {
+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 &IO, FormatStyle::BinaryOperatorStyle &Value) {
IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -323,6 +331,7 @@
Style.AlwaysBreakTemplateDeclarations);
IO.mapOptional("BinPackArguments", Style.BinPackArguments);
IO.mapOptional("BinPackParameters", Style.BinPackParameters);
+IO.mapOptional("BinPackObjCProtocolList", Style.BinPackObjCProtocolList);
IO.mapOptional("BraceWrapping", Style.BraceWrapping);
IO.mapOptional("BreakBeforeBinaryOperators",
Style.BreakBeforeBinaryOperators);
@@ -599,6 +608,7 @@
LLVMStyle.AlwaysBreakTemplateDeclarations = false;
LLVMStyle.BinPackArguments = true;
LLVMStyle.BinPackParameters = true;
+ LLVMStyle.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
LLVMStyle.BreakBeforeTernaryOperators = true;
LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
@@ -752,6 +762,7 @@
GoogleStyle.SpacesInContainerLiterals = false;
} else if (Language == FormatStyle::LK_ObjC) {
GoogleStyle.ColumnLimit = 100;
+GoogleStyle.BinPackObjCProtocolList = FormatStyle::BPS_Never;
}
return GoogleStyle;
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 BinPackObjCProtocolList is unspecified, fall back to BinPackParameters
+// for backwards compatibility.
+bool BinPackObjCProtocolList =
+(Style.BinPackObjCProtocolList == FormatStyle::BPS_Auto &&
+ Style.BinPack
[PATCH] D42650: [clang-format] New format param BinPackObjCProtocolList
benhamilton created this revision.
benhamilton added reviewers: krasimir, jolesiak, stephanemoore.
This is an alternative approach to https://reviews.llvm.org/D42014 after some
investigation by stephanemoore@ and myself.
Previously, the format parameter `BinPackParameters` controlled both
C function parameter list bin-packing and Objective-C protocol conformance
list bin-packing.
We found in the Google style, some teams were changing
`BinPackParameters` from its default (`true`) to `false` so they could
lay out Objective-C protocol conformance list items one-per-line
instead of bin-packing them into as few lines as possible.
To allow teams to use one-per-line Objective-C protocol lists without
changing bin-packing for other areas like C function parameter lists,
this diff introduces a new LibFormat parameter
`BinPackObjCProtocolList` to control the behavior just for ObjC
protocol conformance lists.
The new parameter is an enum which defaults to `Auto` to keep the
previous behavior (delegating to `BinPackParameters`).
For the `google` style, we set it to `Never` so we use one-per-line
protocol conformance lists instead of bin-packing.
Depends On https://reviews.llvm.org/D42649
Test Plan: New tests added. make -j12 FormatTests &&
./tools/clang/unittests/Format/FormatTests
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.BinPackObjCProtocolList = FormatStyle::BPS_Never;
verifyFormat("@interface d () <\n"
"d,\n"
"d,\n"
"d,\n"
"d> {\n"
"}");
+ Style.BinPackParameters = false;
+ Style.BinPackObjCProtocolList = FormatStyle::BPS_Auto;
+ verifyFormat("@interface e () <\n"
+ "e,\n"
+ "e,\n"
+ "e,\n"
+ "e> {\n"
+ "}");
+ Style.BinPackObjCProtocolList = 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"
@@ -309,13 +322,16 @@
verifyFormat("@interface Foo (HackStuff) \n"
"+ (id)init;\n"
"@end");
- Style.BinPackParameters = false;
- Style.ColumnLimit = 80;
- verifyFormat("@interface a () <\n"
- "a,\n"
- ",\n"
- "aa,\n"
- "> {\n"
+ Style.ColumnLimit = 40;
+ // BinPackParameters should be true by default.
+ verifyFormat("void (int e, int e,\n"
+ " int e, int e);\n");
+ // BinPackObjCProtocolList should be BPS_Never by default.
+ verifyFormat("@interface f () <\n"
+ "f,\n"
+ "f,\n"
+ "f,\n"
+ "f> {\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 &IO, FormatStyle::BinPackStyle &Value) {
+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 &IO, FormatStyle::BinaryOperatorStyle &Value) {
IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -323,6 +331,7 @@
Style.AlwaysBreakTemplateDeclarations);
IO.mapOptional("BinPackArguments", Style.BinPackArguments);
IO.mapOptional("BinPackParameters", Style.BinPackParameters);
+IO.mapOptional("BinPackObjCProtocolList", Style.BinPackObjCProtocolList);
IO.mapOptional("BraceWrapping", Style.BraceWrapping);
IO.mapOptional("BreakBeforeBinaryOperators",
Style.BreakBeforeBinaryOperators);
@@ -599,6 +608,7 @@
