On Wed, Oct 23, 2013 at 3:44 PM, Curdeius Curdeius <[email protected]>wrote:
> Hi Daniel,
> thanks for your feedback and sorry for the late response.
>
> I attach the patch.
>
> My comments to your remarks below.
>
> * How is the new test in FormatsWithWebKitStyle related?
>>
>
> I removed the unnecessary mess.
>
>
>> * Either collapse both parameters into a single parameter or add test
>> cases for all combinations.
>>
>
> I did so.
>
Not really. You are still not testing ..BeforeColon = false and
..BeforeComma = true. I agree that it does not make much sense, but then,
an enum containing exactly the three configuration values seems to be a
better solution. You can take a look at UseTabStyle for an example of how
to do that. I imagine an enum like:
enum InheritanceBreakingStyle {
IBS_AfterColon,
IBS_BeforeColon,
IBS_BeforeComma
};
Does that make sense?
>
>> * The parameters themselves have tests (ParsesConfiguration).
>>
>
> Ok, done.
>
>
>>
>> + case tok::semi:
>>> + Contexts.back().CanBeInInheritanceList = false;
>>> + break;
>>
>>
>> Is this necessary? Can you create a test that breaks without it?
>>
>
> You're right. It wasn't necessary
>
>
>>
>> case tok::l_brace:
>>> + Contexts.back().CanBeInInheritanceList = false;
>>> if (!parseBrace())
>>
>>
>> Is this necessary? Can you create a test that breaks without it?
>>
>
> You're right again. It wasn't necessary.
>
>
>>
>> + //} else if (Contexts.size() == 1) {
>>> + // FIXME: annotates switch-case colon as inheritance colon
>>> + // FIXME: annotates access specifiers as inheritance colon
>>> + // Tok->Type = TT_OtherColon;
>>
>>
>> Is this intentional?
>>
>
> I removed this part.
>
>
>>
>> else if (Previous.Type == TT_InheritanceColon)
>>
>> - State.Stack.back().Indent = State.Column;
>>> + // We had a colon on a newline. Now, if we want to align commas
>>> to the
>>> + // colon, we indent less by -2 == length(", ")
>>
>> + if (Style.BreakClassInheritanceListsBeforeComma)
>>> + State.Stack.back().Indent = State.Column - 2;
>>> + else
>>> + State.Stack.back().Indent = State.Column;
>>
>>
>> Remove this completely.
>>
>
> Well, I couldn't remove it completely.
> If you have any idea how to do it in a cleaner way, I'll be glad to know.
>
Yes, you could have. By now, there is one more statement (changing
LastSpace), but everything that was in there could have been removed. As
this is getting too complicated to explain, I have attached a patch
removing everything that I think can be removed (and the tests still pass).
Also, I have cleaned up a few basic formatting errors. Interestingly, you
don't seem to be using clang-format ;-).
>> + if (Current.Type == TT_InheritanceColon) {
>>> State.Stack.back().AvoidBinPacking = true;
>>> + if (!Style.BreakClassInheritanceListsBeforeComma)
>>> + State.Stack.back().Indent = State.Column + 2;
>>> + }
>>
>>
>> Change this to
>>
>> if (Current.Type == TT_InheritanceColon) {
>> State.Stack.back().AvoidBinPacking = true;
>> State.Stack.back().Indent = State.Column;
>> if (!Style.BreakClassInheritanceListsBeforeComma)
>> State.Stack.back().Indent += 2;
>> }
>>
>
> Done.
>
>
>> On Fri, Aug 2, 2013 at 10:59 AM, Curdeius Curdeius
>> <[email protected]>wrote:
>>
>>> Hi,
>>>
>>> I've added two options to clang-format:
>>> * BreakClassInheritanceListsBeforeColon
>>> and
>>> * BreakClassInheritanceListsBeforeComma.
>>>
>>> They allow us to break on class inheritance lists similarly to
>>> constructor initializer lists.
>>>
>>> I've corrected as well the TokenAnnotator, which mistakenly annotated
>>> colons from switch statements ("case:") and from access modifiers
>>> ("public:") as TT_InheritanceColon.
>>>
>>> The patch in the attachment.
>>>
>>> Cheers,
>>> Marek Kurdej
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> [email protected]
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
> Regards,
> Marek Kurdej
>
+ } else if (Current.Type == TT_InheritanceComma) {
+ if (Style.BreakClassInheritanceListsBeforeComma)
+ State.Stack.back().Indent -= 2; // 2 is length of ", "
+ }
I also removed this as it seemed suspicious .. Tests still pass :-).
Please start from the attached patch when continuing to work on this.
Cheers,
Daniel
diff --git a/include/clang/Format/Format.h b/include/clang/Format/Format.h
index 6745082..13f6907 100644
--- a/include/clang/Format/Format.h
+++ b/include/clang/Format/Format.h
@@ -183,6 +183,14 @@ struct FormatStyle {
/// \brief If \c true, binary operators will be placed after line breaks.
bool BreakBeforeBinaryOperators;
+ /// \brief If \c true, colons in class inheritance lists will be placed after
+ /// line breaks.
+ bool BreakClassInheritanceListsBeforeColon;
+
+ /// \brief If \c true, commas in class inheritance lists will be placed after
+ /// line breaks.
+ bool BreakClassInheritanceListsBeforeComma;
+
/// \brief Different ways to attach braces to their surrounding context.
enum BraceBreakingStyle {
/// Always attach braces to surrounding context.
@@ -256,6 +264,10 @@ struct FormatStyle {
BinPackParameters == R.BinPackParameters &&
BreakBeforeBinaryOperators == R.BreakBeforeBinaryOperators &&
BreakBeforeBraces == R.BreakBeforeBraces &&
+ BreakClassInheritanceListsBeforeColon ==
+ R.BreakClassInheritanceListsBeforeColon &&
+ BreakClassInheritanceListsBeforeComma ==
+ R.BreakClassInheritanceListsBeforeComma &&
BreakConstructorInitializersBeforeComma ==
R.BreakConstructorInitializersBeforeComma &&
ColumnLimit == R.ColumnLimit &&
diff --git a/lib/Format/ContinuationIndenter.cpp b/lib/Format/ContinuationIndenter.cpp
index 16b1147..f719014 100644
--- a/lib/Format/ContinuationIndenter.cpp
+++ b/lib/Format/ContinuationIndenter.cpp
@@ -121,6 +121,18 @@ bool ContinuationIndenter::mustBreak(const LineState &State) {
if (State.Stack.back().BreakBeforeClosingBrace &&
Current.closesBlockTypeList(Style))
return true;
+ if (Style.BreakClassInheritanceListsBeforeColon) {
+ if (Previous.Type == TT_InheritanceColon)
+ return false;
+ if (Current.Type == TT_InheritanceColon)
+ return true;
+ }
+ if (Style.BreakClassInheritanceListsBeforeComma) {
+ if (Previous.Type == TT_InheritanceComma)
+ return false;
+ if (Current.Type == TT_InheritanceComma)
+ return true;
+ }
if (Previous.is(tok::semi) && State.LineContainsContinuedForLoopSection)
return true;
if ((startsNextParameter(Current, Style) || Previous.is(tok::semi) ||
@@ -281,10 +293,9 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
// simple assignment without binary expression on the RHS. Also indent
// relative to unary operators and the colons of constructor initializers.
State.Stack.back().LastSpace = State.Column;
- else if (Previous.Type == TT_InheritanceColon) {
- State.Stack.back().Indent = State.Column;
+ else if (Previous.Type == TT_InheritanceColon)
State.Stack.back().LastSpace = State.Column;
- } else if (Previous.opensScope()) {
+ else if (Previous.opensScope()) {
// If a function has a trailing call, indent all parameters from the
// opening parenthesis. This avoids confusing indents like:
// OuterFunction(InnerFunctionCall( // break
@@ -450,8 +461,12 @@ unsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
const FormatToken &Current = *State.NextToken;
assert(State.Stack.size());
- if (Current.Type == TT_InheritanceColon)
+ if (Current.Type == TT_InheritanceColon) {
State.Stack.back().AvoidBinPacking = true;
+ State.Stack.back().Indent = State.Column;
+ if (!Style.BreakClassInheritanceListsBeforeComma)
+ State.Stack.back().Indent += 2; // 2 is length of ": "
+ }
if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == 0)
State.Stack.back().FirstLessLess = State.Column;
if (Current.Type == TT_ArraySubscriptLSquare &&
diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp
index 79244b1..58c0106 100644
--- a/lib/Format/Format.cpp
+++ b/lib/Format/Format.cpp
@@ -21,7 +21,7 @@
#include "WhitespaceManager.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/SourceManager.h"
-#include "clang/Format/Format.h"
+#include "clang/Format/Format.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Allocator.h"
@@ -123,6 +123,10 @@ template <> struct MappingTraits<clang::format::FormatStyle> {
Style.AlwaysBreakBeforeMultilineStrings);
IO.mapOptional("BreakBeforeBinaryOperators",
Style.BreakBeforeBinaryOperators);
+ IO.mapOptional("BreakClassInheritanceListsBeforeColon",
+ Style.BreakClassInheritanceListsBeforeColon);
+ IO.mapOptional("BreakClassInheritanceListsBeforeComma",
+ Style.BreakClassInheritanceListsBeforeComma);
IO.mapOptional("BreakConstructorInitializersBeforeComma",
Style.BreakConstructorInitializersBeforeComma);
IO.mapOptional("BinPackParameters", Style.BinPackParameters);
@@ -192,6 +196,8 @@ FormatStyle getLLVMStyle() {
LLVMStyle.BinPackParameters = true;
LLVMStyle.BreakBeforeBinaryOperators = false;
LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
+ LLVMStyle.BreakClassInheritanceListsBeforeColon = false;
+ LLVMStyle.BreakClassInheritanceListsBeforeComma = false;
LLVMStyle.BreakConstructorInitializersBeforeComma = false;
LLVMStyle.ColumnLimit = 80;
LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
@@ -236,6 +242,8 @@ FormatStyle getGoogleStyle() {
GoogleStyle.BinPackParameters = true;
GoogleStyle.BreakBeforeBinaryOperators = false;
GoogleStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
+ GoogleStyle.BreakClassInheritanceListsBeforeColon = false;
+ GoogleStyle.BreakClassInheritanceListsBeforeComma = false;
GoogleStyle.BreakConstructorInitializersBeforeComma = false;
GoogleStyle.ColumnLimit = 80;
GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
diff --git a/lib/Format/FormatToken.h b/lib/Format/FormatToken.h
index 2001d42..ec9b6f9 100644
--- a/lib/Format/FormatToken.h
+++ b/lib/Format/FormatToken.h
@@ -38,6 +38,7 @@ enum TokenType {
TT_ImplicitStringLiteral,
TT_InlineASMColon,
TT_InheritanceColon,
+ TT_InheritanceComma,
TT_FunctionTypeLParen,
TT_LambdaLSquare,
TT_LineComment,
diff --git a/lib/Format/TokenAnnotator.cpp b/lib/Format/TokenAnnotator.cpp
index e398442..ff8f14a 100644
--- a/lib/Format/TokenAnnotator.cpp
+++ b/lib/Format/TokenAnnotator.cpp
@@ -418,6 +418,13 @@ private:
case tok::question:
parseConditional();
break;
+ case tok::kw_class:
+ case tok::kw_struct:
+ // A colon ':' before a left brace (class definition start) or
+ // a semicolon (class forward declaration end) will start an inheritance
+ // list.
+ Contexts.back().CanBeInInheritanceList = true;
+ break;
case tok::kw_template:
parseTemplateDeclaration();
break;
@@ -431,6 +438,8 @@ private:
Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
if (Contexts.back().InCtorInitializer)
Tok->Type = TT_CtorInitializerComma;
+ if (Contexts.back().CanBeInInheritanceList)
+ Tok->Type = TT_InheritanceComma;
break;
default:
break;
@@ -559,11 +568,12 @@ private:
ColonIsObjCDictLiteral(false), ColonIsObjCMethodExpr(false),
FirstObjCSelectorName(NULL), FirstStartOfName(NULL),
IsExpression(IsExpression), CanBeExpression(true),
- InCtorInitializer(false) {}
+ InCtorInitializer(false), CanBeInInheritanceList(false) {}
tok::TokenKind ContextKind;
unsigned BindingStrength;
unsigned LongestObjCSelectorName;
+ bool CanBeInInheritanceList;
bool ColonIsForRangeExpr;
bool ColonIsObjCDictLiteral;
bool ColonIsObjCMethodExpr;
diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index d8ff2fc..bc7b8f1 100644
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -2618,13 +2618,13 @@ TEST_F(FormatTest, ExpressionIndentationBreakingBeforeOperators) {
TEST_F(FormatTest, ConstructorInitializers) {
verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
- verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
+ verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}",
getLLVMStyleWithColumns(45));
verifyFormat("Constructor()\n"
- " : Inttializer(FitsOnTheLine) {}",
+ " : Initializer(FitsOnTheLine) {}",
getLLVMStyleWithColumns(44));
verifyFormat("Constructor()\n"
- " : Inttializer(FitsOnTheLine) {}",
+ " : Initializer(FitsOnTheLine) {}",
getLLVMStyleWithColumns(43));
verifyFormat(
@@ -6544,6 +6544,8 @@ TEST_F(FormatTest, ParsesConfiguration) {
CHECK_PARSE_BOOL(BinPackParameters);
CHECK_PARSE_BOOL(BreakBeforeBinaryOperators);
CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
+ CHECK_PARSE_BOOL(BreakClassInheritanceListsBeforeColon);
+ CHECK_PARSE_BOOL(BreakClassInheritanceListsBeforeComma);
CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
CHECK_PARSE_BOOL(DerivePointerBinding);
CHECK_PARSE_BOOL(IndentCaseLabels);
@@ -6968,5 +6970,80 @@ TEST_F(FormatTest, ConfigurableContinuationIndentWidth) {
format("int i = longFunction(arg);", SixIndent));
}
+TEST_F(FormatTest, BreaksBeforeClassInheritanceColon) {
+ FormatStyle Style = getWebKitStyle();
+ Style.BreakClassInheritanceListsBeforeColon = true;
+
+ verifyFormat("namespace outer {\n"
+ "int i;\n"
+ "namespace inner {\n"
+ " int i;\n"
+ " class Aaaa\n"
+ " : public Bbb,\n"
+ " protected Ccc {\n"
+ " Aaaa()\n"
+ " : Bbb(aaaa)\n"
+ " , Ccc(aaaa)\n"
+ " {\n"
+ " }\n"
+ " };\n"
+ "} // namespace inner\n"
+ "} // namespace outer\n"
+ "namespace other_outer {\n"
+ "int i;\n"
+ "}",
+ Style);
+}
+
+TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) {
+ FormatStyle Style = getWebKitStyle();
+ Style.BreakClassInheritanceListsBeforeComma = true;
+
+ verifyFormat("namespace outer {\n"
+ "int i;\n"
+ "namespace inner {\n"
+ " int i;\n"
+ " class Aaaa : public Bbb\n"
+ " , protected Ccc {\n"
+ " Aaaa()\n"
+ " : Bbb(aaaa)\n"
+ " , Ccc(aaaa)\n"
+ " {\n"
+ " }\n"
+ " };\n"
+ "} // namespace inner\n"
+ "} // namespace outer\n"
+ "namespace other_outer {\n"
+ "int i;\n"
+ "}",
+ Style);
+}
+
+TEST_F(FormatTest, BreaksBeforeClassInheritanceColonAndComma) {
+ FormatStyle Style = getWebKitStyle();
+ Style.BreakClassInheritanceListsBeforeColon = true;
+ Style.BreakClassInheritanceListsBeforeComma = true;
+
+ verifyFormat("namespace outer {\n"
+ "int i;\n"
+ "namespace inner {\n"
+ " int i;\n"
+ " class Aaaa\n"
+ " : public Bbb\n"
+ " , protected Ccc {\n"
+ " Aaaa()\n"
+ " : Bbb(aaaa)\n"
+ " , Ccc(aaaa)\n"
+ " {\n"
+ " }\n"
+ " };\n"
+ "} // namespace inner\n"
+ "} // namespace outer\n"
+ "namespace other_outer {\n"
+ "int i;\n"
+ "}",
+ Style);
+}
+
} // end namespace tooling
} // end namespace clang
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
