r195256 - Added an option to allow short function bodies be placed on a single line.

Daniel Jasper djasper at google.com
Wed Nov 20 09:12:58 PST 2013


On Wed, Nov 20, 2013 at 8:33 AM, Alexander Kornienko <alexfh at google.com>wrote:

> Author: alexfh
> Date: Wed Nov 20 10:33:05 2013
> New Revision: 195256
>
> URL: http://llvm.org/viewvc/llvm-project?rev=195256&view=rev
> Log:
> Added an option to allow short function bodies be placed on a single line.
>
> Summary:
> The AllowShortFunctionsOnASingleLine option now controls short function
> body placement on a single line independent of the BreakBeforeBraces
> option.
> Updated tests using BreakBeforeBraces other than BS_Attach.
>
> Addresses http://llvm.org/PR17888
>
> Reviewers: klimek, djasper
>
> Reviewed By: klimek
>
> CC: cfe-commits, klimek
>
> Differential Revision: http://llvm-reviews.chandlerc.com/D2230
>
> Modified:
>     cfe/trunk/include/clang/Format/Format.h
>     cfe/trunk/lib/Format/Format.cpp
>     cfe/trunk/lib/Format/FormatToken.h
>     cfe/trunk/lib/Format/TokenAnnotator.cpp
>     cfe/trunk/lib/Format/UnwrappedLineParser.cpp
>     cfe/trunk/unittests/Format/FormatTest.cpp
>
> Modified: cfe/trunk/include/clang/Format/Format.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Format/Format.h?rev=195256&r1=195255&r2=195256&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Format/Format.h (original)
> +++ cfe/trunk/include/clang/Format/Format.h Wed Nov 20 10:33:05 2013
> @@ -141,6 +141,10 @@ struct FormatStyle {
>    /// single line.
>    bool AllowShortLoopsOnASingleLine;
>
> +  /// \brief If \c true, <tt>int f() { return 0; }</tt> can be put on a
> single
> +  /// line.
> +  bool AllowShortFunctionsOnASingleLine;
> +
>    /// \brief Add a space in front of an Objective-C protocol list, i.e.
> use
>    /// <tt>Foo <Protocol></tt> instead of \c Foo<Protocol>.
>    bool ObjCSpaceBeforeProtocolList;
> @@ -255,6 +259,8 @@ struct FormatStyle {
>             AlignTrailingComments == R.AlignTrailingComments &&
>             AllowAllParametersOfDeclarationOnNextLine ==
>                 R.AllowAllParametersOfDeclarationOnNextLine &&
> +           AllowShortFunctionsOnASingleLine ==
> +               R.AllowShortFunctionsOnASingleLine &&
>             AllowShortIfStatementsOnASingleLine ==
>                 R.AllowShortIfStatementsOnASingleLine &&
>             AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine
> &&
>
> Modified: cfe/trunk/lib/Format/Format.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=195256&r1=195255&r2=195256&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Format/Format.cpp (original)
> +++ cfe/trunk/lib/Format/Format.cpp Wed Nov 20 10:33:05 2013
> @@ -117,6 +117,8 @@ template <> struct MappingTraits<clang::
>                     Style.AllowShortIfStatementsOnASingleLine);
>      IO.mapOptional("AllowShortLoopsOnASingleLine",
>                     Style.AllowShortLoopsOnASingleLine);
> +    IO.mapOptional("AllowShortFunctionsOnASingleLine",
> +                   Style.AllowShortFunctionsOnASingleLine);
>      IO.mapOptional("AlwaysBreakTemplateDeclarations",
>                     Style.AlwaysBreakTemplateDeclarations);
>      IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
> @@ -190,6 +192,7 @@ FormatStyle getLLVMStyle() {
>    LLVMStyle.AlignEscapedNewlinesLeft = false;
>    LLVMStyle.AlignTrailingComments = true;
>    LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
> +  LLVMStyle.AllowShortFunctionsOnASingleLine = true;
>    LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
>    LLVMStyle.AllowShortLoopsOnASingleLine = false;
>    LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
> @@ -237,6 +240,7 @@ FormatStyle getGoogleStyle() {
>    GoogleStyle.AlignEscapedNewlinesLeft = true;
>    GoogleStyle.AlignTrailingComments = true;
>    GoogleStyle.AllowAllParametersOfDeclarationOnNextLine = true;
> +  GoogleStyle.AllowShortFunctionsOnASingleLine = true;
>    GoogleStyle.AllowShortIfStatementsOnASingleLine = true;
>    GoogleStyle.AllowShortLoopsOnASingleLine = true;
>    GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
> @@ -381,7 +385,7 @@ public:
>    /// \brief Calculates how many lines can be merged into 1 starting at
> \p I.
>    unsigned
>    tryFitMultipleLinesInOne(unsigned Indent,
> -                           SmallVectorImpl<AnnotatedLine
> *>::const_iterator &I,
> +                           SmallVectorImpl<AnnotatedLine
> *>::const_iterator I,
>                             SmallVectorImpl<AnnotatedLine
> *>::const_iterator E) {
>      // We can never merge stuff if there are trailing line comments.
>      AnnotatedLine *TheLine = *I;
> @@ -402,16 +406,43 @@ public:
>      if (I + 1 == E || I[1]->Type == LT_Invalid)
>        return 0;
>
> +    if (TheLine->Last->Type == TT_FunctionLBrace) {
> +      return Style.AllowShortFunctionsOnASingleLine
> +                 ? tryMergeSimpleBlock(I, E, Limit)
> +                 : 0;
> +    }
>      if (TheLine->Last->is(tok::l_brace)) {
> -      return tryMergeSimpleBlock(I, E, Limit);
> -    } else if (Style.AllowShortIfStatementsOnASingleLine &&
> -               TheLine->First->is(tok::kw_if)) {
> -      return tryMergeSimpleControlStatement(I, E, Limit);
> -    } else if (Style.AllowShortLoopsOnASingleLine &&
> -               TheLine->First->isOneOf(tok::kw_for, tok::kw_while)) {
> -      return tryMergeSimpleControlStatement(I, E, Limit);
> -    } else if (TheLine->InPPDirective &&
> (TheLine->First->HasUnescapedNewline ||
> -                                          TheLine->First->IsFirst)) {
> +      return Style.BreakBeforeBraces ==
> clang::format::FormatStyle::BS_Attach
> +                 ? tryMergeSimpleBlock(I, E, Limit)
> +                 : 0;
> +    }
> +    if (I[1]->First->Type == TT_FunctionLBrace &&
> +        Style.BreakBeforeBraces != FormatStyle::BS_Attach) {
> +      // Reduce the column limit by the number of spaces we need to insert
> +      // around braces.
> +      Limit = Limit > 3 ? Limit - 3 : 0;
> +      unsigned MergedLines = 0;
> +      if (Style.AllowShortFunctionsOnASingleLine) {
> +        MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
> +        // If we managed to merge the block, count the function header,
> which is
> +        // on a separate line.
> +        if (MergedLines > 0)
> +          ++MergedLines;
> +      }
> +      return MergedLines;
> +    }
> +    if (TheLine->First->is(tok::kw_if)) {
> +      return Style.AllowShortIfStatementsOnASingleLine
> +                 ? tryMergeSimpleControlStatement(I, E, Limit)
> +                 : 0;
> +    }
> +    if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while)) {
> +      return Style.AllowShortLoopsOnASingleLine
> +                 ? tryMergeSimpleControlStatement(I, E, Limit)
> +                 : 0;
> +    }
> +    if (TheLine->InPPDirective &&
> +        (TheLine->First->HasUnescapedNewline || TheLine->First->IsFirst))
> {
>        return tryMergeSimplePPDirective(I, E, Limit);
>      }
>      return 0;
> @@ -419,7 +450,7 @@ public:
>
>  private:
>    unsigned
> -  tryMergeSimplePPDirective(SmallVectorImpl<AnnotatedLine
> *>::const_iterator &I,
> +  tryMergeSimplePPDirective(SmallVectorImpl<AnnotatedLine
> *>::const_iterator I,
>                              SmallVectorImpl<AnnotatedLine
> *>::const_iterator E,
>                              unsigned Limit) {
>      if (Limit == 0)
> @@ -434,7 +465,7 @@ private:
>    }
>
>    unsigned tryMergeSimpleControlStatement(
> -      SmallVectorImpl<AnnotatedLine *>::const_iterator &I,
> +      SmallVectorImpl<AnnotatedLine *>::const_iterator I,
>        SmallVectorImpl<AnnotatedLine *>::const_iterator E, unsigned Limit)
> {
>      if (Limit == 0)
>        return 0;
> @@ -450,7 +481,7 @@ private:
>      if (1 + I[1]->Last->TotalLength > Limit)
>        return 0;
>      if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for,
> -                                   tok::kw_while) ||
> +                             tok::kw_while) ||
>          I[1]->First->Type == TT_LineComment)
>        return 0;
>      // Only inline simple if's (no nested if or else).
> @@ -461,13 +492,9 @@ private:
>    }
>
>    unsigned
> -  tryMergeSimpleBlock(SmallVectorImpl<AnnotatedLine *>::const_iterator &I,
> +  tryMergeSimpleBlock(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
>                        SmallVectorImpl<AnnotatedLine *>::const_iterator E,
>                        unsigned Limit) {
> -    // No merging if the brace already is on the next line.
> -    if (Style.BreakBeforeBraces != FormatStyle::BS_Attach)
> -      return 0;
> -
>      // First, check that the current line allows merging. This is the
> case if
>      // we're not in a control flow statement and the last token is an
> opening
>      // brace.
> @@ -583,8 +610,7 @@ public:
>          }
>        } else if (TheLine.Type != LT_Invalid &&
>                   (WasMoved || FormatPPDirective || touchesLine(TheLine)))
> {
> -        unsigned LevelIndent =
> -            getIndent(IndentForLevel, TheLine.Level);
> +        unsigned LevelIndent = getIndent(IndentForLevel, TheLine.Level);
>          if (FirstTok->WhitespaceRange.isValid()) {
>            if (!DryRun)
>              formatFirstToken(*TheLine.First, PreviousLine, TheLine.Level,
> @@ -732,9 +758,9 @@ private:
>      if (PreviousLine && PreviousLine->First->isAccessSpecifier())
>        Newlines = std::min(1u, Newlines);
>
> -    Whitespaces->replaceWhitespace(
> -        RootToken, Newlines, IndentLevel, Indent, Indent,
> -        InPPDirective && !RootToken.HasUnescapedNewline);
> +    Whitespaces->replaceWhitespace(RootToken, Newlines, IndentLevel,
> Indent,
> +                                   Indent, InPPDirective &&
> +
> !RootToken.HasUnescapedNewline);
>    }
>
>    /// \brief Get the indent of \p Level from \p IndentForLevel.
> @@ -961,7 +987,7 @@ private:
>
>      // Cannot merge multiple statements into a single line.
>      if (Previous.Children.size() > 1)
> -      return false;
> +      return false;
>
>      // We can't put the closing "}" on a line with a trailing comment.
>      if (Previous.Children[0]->Last->isTrailingComment())
>
> Modified: cfe/trunk/lib/Format/FormatToken.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/FormatToken.h?rev=195256&r1=195255&r2=195256&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Format/FormatToken.h (original)
> +++ cfe/trunk/lib/Format/FormatToken.h Wed Nov 20 10:33:05 2013
> @@ -39,6 +39,7 @@ enum TokenType {
>    TT_ImplicitStringLiteral,
>    TT_InlineASMColon,
>    TT_InheritanceColon,
> +  TT_FunctionLBrace,
>    TT_FunctionTypeLParen,
>    TT_LambdaLSquare,
>    TT_LineComment,
>
> Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=195256&r1=195255&r2=195256&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
> +++ cfe/trunk/lib/Format/TokenAnnotator.cpp Wed Nov 20 10:33:05 2013
> @@ -538,6 +538,7 @@ private:
>        // Reset token type in case we have already looked at it and then
>        // recovered from an error (e.g. failure to find the matching >).
>        if (CurrentToken->Type != TT_LambdaLSquare &&
> +          CurrentToken->Type != TT_FunctionLBrace &&
>            CurrentToken->Type != TT_ImplicitStringLiteral)
>          CurrentToken->Type = TT_Unknown;
>        if (CurrentToken->Role)
>
> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=195256&r1=195255&r2=195256&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Wed Nov 20 10:33:05 2013
> @@ -681,6 +681,7 @@ void UnwrappedLineParser::parseStructura
>              Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup ||
>              Style.BreakBeforeBraces == FormatStyle::BS_Allman)
>            addUnwrappedLine();
>

Does it still make sense to report the "{" as its own unwrapped line? Seems
a bit convoluted to first report multiple lines and then merge them
afterwards. I think this would make the merging code simpler.


> +        FormatTok->Type = TT_FunctionLBrace;
>          parseBlock(/*MustBeDeclaration=*/false);
>          addUnwrappedLine();
>          return;
>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=195256&r1=195255&r2=195256&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Nov 20 10:33:05 2013
> @@ -4776,8 +4776,15 @@ TEST_F(FormatTest, FormatsBracedListsInC
>  }
>
>  TEST_F(FormatTest, PullTrivialFunctionDefinitionsIntoSingleLine) {
> +  FormatStyle DoNotMerge = getLLVMStyle();
> +  DoNotMerge.AllowShortFunctionsOnASingleLine = false;
> +
>    verifyFormat("void f() { return 42; }");
>    verifyFormat("void f() {\n"
> +               "  return 42;\n"
> +               "}",
> +               DoNotMerge);
> +  verifyFormat("void f() {\n"
>                 "  // Comment\n"
>                 "}");
>    verifyFormat("{\n"
> @@ -4792,6 +4799,13 @@ TEST_F(FormatTest, PullTrivialFunctionDe
>    verifyFormat("void f() { int a; } // comment");
>    verifyFormat("void f() {\n"
>                 "} // comment",
> +               DoNotMerge);
> +  verifyFormat("void f() {\n"
> +               "  int a;\n"
> +               "} // comment",
> +               DoNotMerge);
> +  verifyFormat("void f() {\n"
> +               "} // comment",
>                 getLLVMStyleWithColumns(15));
>
>    verifyFormat("void f() { return 42; }", getLLVMStyleWithColumns(23));
> @@ -6613,10 +6627,7 @@ TEST_F(FormatTest, LinuxBraceBreaking) {
>                 "      b();\n"
>                 "    }\n"
>                 "  }\n"
> -               "  void g()\n"
> -               "  {\n"
> -               "    return;\n"
> -               "  }\n"
> +               "  void g() { return; }\n"
>                 "}\n"
>                 "}",
>                 BreakBeforeBrace);
> @@ -6634,10 +6645,7 @@ TEST_F(FormatTest, StroustrupBraceBreaki
>                 "      b();\n"
>                 "    }\n"
>                 "  }\n"
> -               "  void g()\n"
> -               "  {\n"
> -               "    return;\n"
> -               "  }\n"
> +               "  void g() { return; }\n"
>                 "}\n"
>                 "}",
>                 BreakBeforeBrace);
> @@ -6658,10 +6666,7 @@ TEST_F(FormatTest, AllmanBraceBreaking)
>                 "      b();\n"
>                 "    }\n"
>                 "  }\n"
> -               "  void g()\n"
> -               "  {\n"
> -               "    return;\n"
> -               "  }\n"
> +               "  void g() { return; }\n"
>                 "}\n"
>                 "}",
>                 BreakBeforeBrace);
> @@ -6822,6 +6827,7 @@ TEST_F(FormatTest, ParsesConfiguration)
>    CHECK_PARSE_BOOL(AlignEscapedNewlinesLeft);
>    CHECK_PARSE_BOOL(AlignTrailingComments);
>    CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
> +  CHECK_PARSE_BOOL(AllowShortFunctionsOnASingleLine);
>    CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
>    CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
>    CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
> @@ -7126,7 +7132,7 @@ TEST_F(FormatTest, FormatsWithWebKitStyl
>                 "    :
> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
>                 "    , aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaa, // break\n"
>                 "                               aaaaaaaaaaaaaa)\n"
> -               "    , aaaaaaaaaaaaaaaaaaaaaaa()\n{\n}",
> +               "    , aaaaaaaaaaaaaaaaaaaaaaa() {}",
>                 Style);
>
>    // Access specifiers should be aligned left.
>
>
> _______________________________________________
> 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/20131120/065810ec/attachment.html>


More information about the cfe-commits mailing list