r178542 - Fix some inconsistent use of indentation.
Manuel Klimek
klimek at google.com
Tue Apr 2 07:54:59 PDT 2013
On Tue, Apr 2, 2013 at 4:33 PM, Daniel Jasper <djasper at google.com> wrote:
> Author: djasper
> Date: Tue Apr 2 09:33:13 2013
> New Revision: 178542
>
> URL: http://llvm.org/viewvc/llvm-project?rev=178542&view=rev
> Log:
> Fix some inconsistent use of indentation.
>
> Basically we have always special-cased the top-level statement of an
> unwrapped line (the one with ParenLevel == 0) and that lead to several
> inconsistencies. All added tests were formatted in a strange way, for
> example:
>
> Before:
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();
> if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()) {
> }
>
> After:
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();
> if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()) {
> }
>
> Modified:
> cfe/trunk/lib/Format/Format.cpp
> cfe/trunk/unittests/Format/FormatTest.cpp
>
> Modified: cfe/trunk/lib/Format/Format.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=178542&r1=178541&r2=178542&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Format/Format.cpp (original)
> +++ cfe/trunk/lib/Format/Format.cpp Tue Apr 2 09:33:13 2013
> @@ -485,7 +485,7 @@ public:
> State.Column = FirstIndent;
> State.NextToken = &RootToken;
> State.Stack.push_back(
> - ParenState(FirstIndent + 4, FirstIndent, !Style.BinPackParameters,
> + ParenState(FirstIndent, FirstIndent, !Style.BinPackParameters,
> /*HasMultiParameterLine=*/ false));
> State.VariablePos = 0;
> State.LineContainsContinuedForLoopSection = false;
> @@ -536,7 +536,8 @@ private:
> BreakBeforeClosingBrace(false), QuestionColumn(0),
> AvoidBinPacking(AvoidBinPacking), BreakBeforeParameter(false),
> HasMultiParameterLine(HasMultiParameterLine), ColonPos(0),
> - StartOfFunctionCall(0) {}
> + StartOfFunctionCall(0), NestedNameSpecifierContinuation(0),
> + CallContinuation(0) {}
>
> /// \brief The position to which a specific parenthesis level needs
> to be
> /// indented.
> @@ -582,6 +583,14 @@ private:
> /// \brief The start of the most recent function in a builder-type
> call.
> unsigned StartOfFunctionCall;
>
> + /// \brief If a nested name specifier was broken over multiple lines,
> this
> + /// contains the start column of the second line. Otherwise 0.
> + unsigned NestedNameSpecifierContinuation;
> +
> + /// \brief If a call expression was broken over multiple lines, this
> + /// contains the start column of the second line. Otherwise 0.
> + unsigned CallContinuation;
> +
> bool operator<(const ParenState &Other) const {
> if (Indent != Other.Indent)
> return Indent < Other.Indent;
> @@ -603,6 +612,12 @@ private:
> return ColonPos < Other.ColonPos;
> if (StartOfFunctionCall != Other.StartOfFunctionCall)
> return StartOfFunctionCall < Other.StartOfFunctionCall;
> + if (NestedNameSpecifierContinuation !=
> + Other.NestedNameSpecifierContinuation)
> + return NestedNameSpecifierContinuation <
> + Other.NestedNameSpecifierContinuation;
> + if (CallContinuation != Other.CallContinuation)
> + return CallContinuation < Other.CallContinuation;
> return false;
> }
> };
> @@ -682,6 +697,8 @@ private:
> return 0;
> }
>
> + unsigned IndentedFurther =
>
I'd call it "ContinuationIndent"
> + std::max(State.Stack.back().LastSpace, State.Stack.back().Indent)
> + 4;
> if (Newline) {
> unsigned WhitespaceStartColumn = State.Column;
> if (Current.is(tok::r_brace)) {
> @@ -693,15 +710,16 @@ private:
> } else if (Current.is(tok::lessless) &&
> State.Stack.back().FirstLessLess != 0) {
> State.Column = State.Stack.back().FirstLessLess;
> - } else if (State.ParenLevel != 0 &&
> - (Previous.isOneOf(tok::equal, tok::coloncolon) ||
> - Current.isOneOf(tok::period, tok::arrow, tok::question)
> ||
> - isComparison(Previous))) {
> - // Indent and extra 4 spaces after if we know the current
> expression is
> - // continued. Don't do that on the top level, as we already
> indent 4
> - // there.
> - State.Column = std::max(State.Stack.back().LastSpace,
> - State.Stack.back().Indent) + 4;
> + } else if (Previous.is(tok::coloncolon)) {
>
I think this is easy to understand for me as I saw you scribbling on the
whiteboard, but I'd add a comment like // Continuation of a broken nested
name specifier.
> + State.Column = IndentedFurther;
> + if (State.Stack.back().NestedNameSpecifierContinuation == 0)
> + State.Stack.back().NestedNameSpecifierContinuation =
> State.Column;
> + State.Column = State.Stack.back().NestedNameSpecifierContinuation;
>
You really wanted to save lines here :) I'd unfold this into two trivially
to understand cases. Then you also don't need the comment, as the first
line I hit will say "NestedNameSpecifierContinuation".
> + } else if (Current.isOneOf(tok::period, tok::arrow)) {
> + State.Column = IndentedFurther;
> + if (State.Stack.back().CallContinuation == 0)
> + State.Stack.back().CallContinuation = State.Column;
> + State.Column = State.Stack.back().CallContinuation;
} else if (Current.Type == TT_ConditionalExpr) {
> State.Column = State.Stack.back().QuestionColumn;
> } else if (Previous.is(tok::comma) && State.VariablePos != 0 &&
> @@ -710,7 +728,7 @@ private:
> State.Column = State.VariablePos;
> } else if (Previous.ClosesTemplateDeclaration ||
> (Current.Type == TT_StartOfName && State.ParenLevel ==
> 0)) {
> - State.Column = State.Stack.back().Indent - 4;
> + State.Column = State.Stack.back().Indent;
> } else if (Current.Type == TT_ObjCSelectorName) {
> if (State.Stack.back().ColonPos > Current.FormatTok.TokenLength) {
> State.Column =
> @@ -720,11 +738,15 @@ private:
> State.Stack.back().ColonPos =
> State.Column + Current.FormatTok.TokenLength;
> }
> - } else if (Previous.Type == TT_ObjCMethodExpr ||
> - Current.Type == TT_StartOfName) {
> - State.Column = State.Stack.back().Indent + 4;
> + } else if (Current.Type == TT_StartOfName ||
> Current.is(tok::question) ||
> + Previous.is(tok::equal) || isComparison(Previous) ||
> + Previous.Type == TT_ObjCMethodExpr) {
> + // Indent and extra 4 spaces if the current expression is
> continued.
>
s/and/an/
> + State.Column = IndentedFurther;
> } else {
> State.Column = State.Stack.back().Indent;
> + if (State.Column == FirstIndent)
>
Can you please explain this in a comment.
> + State.Column += 4;
> }
>
> if (Current.is(tok::question))
> @@ -845,6 +867,8 @@ private:
> State.Stack.back().AvoidBinPacking = true;
> State.Stack.back().BreakBeforeParameter = false;
> }
> + if (Current.Type == TT_ObjCMethodSpecifier)
> + State.Stack.back().Indent += 4;
>
... and this.
>
> // Insert scopes created by fake parenthesis.
> for (unsigned i = 0, e = Current.FakeLParens; i != e; ++i) {
>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=178542&r1=178541&r2=178542&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Tue Apr 2 09:33:13 2013
> @@ -1670,7 +1670,7 @@ TEST_F(FormatTest, FormatsBuilderPattern
> " .Default(ORDER_TEXT);\n");
>
> verifyFormat("return
> aaaaaaaaaaaaaaaaa->aaaaa().aaaaaaaaaaaaa().aaaaaa() <\n"
> - "
> aaaaaaaaaaaaaaaaaaa->aaaaa().aaaaaaaaaaaaa().aaaaaa();");
> + "
> aaaaaaaaaaaaaaa->aaaaa().aaaaaaaaaaaaa().aaaaaa();");
> verifyFormat(
> "aaaaaaa->aaaaaaa\n"
> "
> ->aaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
> @@ -1765,6 +1765,12 @@ TEST_F(FormatTest, AlignsAfterReturn) {
> verifyFormat(
> "return (aaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaa +\n"
> " aaaaaaaaaaaaaaaaaaaaaaaaa);");
> + verifyFormat(
> + "return aaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> >=\n"
> + " aaaaaaaaaaaaaaaaaaaaaa();");
> + verifyFormat(
> + "return (aaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> >=\n"
> + " aaaaaaaaaaaaaaaaaaaaaa());");
> }
>
> TEST_F(FormatTest, BreaksConditionalExpressions) {
> @@ -1799,6 +1805,10 @@ TEST_F(FormatTest, BreaksConditionalExpr
> verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> " ? aaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> " : aaaaaaaaaaaaaaaaaaaaaaaaaaa;");
> + verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaa =\n"
> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " ? aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " : aaaaaaaaaaaaaaaa;");
> verifyFormat(
> "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> " ? aaaaaaaaaaaaaaa\n"
> @@ -1837,7 +1847,7 @@ TEST_F(FormatTest, DeclarationsOfMultipl
> verifyFormat("bool aaaaaaaaaaaaaaaaaaaaaaaaa =\n"
> " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaa),\n"
> " bbbbbbbbbbbbbbbbbbbbbbbbb =\n"
> - " bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb(bbbbbbbbbbbbbbbb);");
> + "
> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb(bbbbbbbbbbbbbbbb);");
> verifyFormat(
> "bool aaaaaaaaaaaaaaaaaaaaa =\n"
> " bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb &&
> cccccccccccccccccccccccccccc,\n"
> @@ -1920,6 +1930,10 @@ TEST_F(FormatTest, AlignsPipes) {
> " << \"eeeeeeeeeeeeeeeee = \" << eeeeeeeeeeeeeeeee;");
> verifyFormat("llvm::outs() << aaaaaaaaaaaaaaaaaaaaaaaa << \"=\"\n"
> " << bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;");
> +
> + verifyFormat(
> + "llvm::errs() << aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();");
> }
>
> TEST_F(FormatTest, UnderstandsEquals) {
> @@ -1973,6 +1987,11 @@ TEST_F(FormatTest, WrapsAtFunctionCallsI
> " .aaaaaaaaaaaaaaa(\n"
> " aa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,
> aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
> " aaaaaaaaaaaaaaaaaaaaaaaaaaa));");
> + verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa()) {\n"
> + "}");
>
> // Here, it is not necessary to wrap at "." or "->".
> verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa)
> ||\n"
> @@ -2068,6 +2087,10 @@ TEST_F(FormatTest, WrapsAtNestedNameSpec
> "
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
> " aaaaaaaaaaaaaaaaaaaaa);",
> getLLVMStyleWithColumns(74));
> +
> + verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa::\n"
> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " .aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa();");
> }
>
> TEST_F(FormatTest, UnderstandsTemplateParameters) {
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130402/849b38f8/attachment.html>
More information about the cfe-commits
mailing list