r356031 - [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present
Roman Lebedev via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 13 01:59:20 PDT 2019
This was reviewed by someone already familiar with, and active with
the code in question?
On Wed, Mar 13, 2019 at 11:25 AM Paul Hoad via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
>
> Author: paulhoad
> Date: Wed Mar 13 01:26:39 2019
> New Revision: 356031
>
> URL: http://llvm.org/viewvc/llvm-project?rev=356031&view=rev
> Log:
> [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present
>
> Summary:
> Addressing: PR25010 - https://bugs.llvm.org/show_bug.cgi?id=25010
>
> Code like:
>
> ```
> if(true) var++;
> else {
> var--;
> }
> ```
>
> is reformatted to be
>
> ```
> if (true)
> var++;
> else {
> var--;
> }
> ```
>
> Even when `AllowShortIfStatementsOnASingleLine` is true
>
> The following revision comes from a +1'd suggestion in the PR to support AllowShortIfElseStatementsOnASingleLine
>
> This suppresses the clause prevents the merging of the if when there is a compound else
>
> Reviewers: klimek, djasper, JonasToth, alexfh, krasimir, reuk
> Reviewed By: reuk
> Subscribers: reuk, Higuoxing, jdoerfert, cfe-commits
> Tags: #clang-tools-extra
> Differential Revision: https://reviews.llvm.org/D59087
>
> Modified:
> cfe/trunk/docs/ClangFormatStyleOptions.rst
> cfe/trunk/include/clang/Format/Format.h
> cfe/trunk/lib/Format/Format.cpp
> cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
> cfe/trunk/unittests/Format/FormatTest.cpp
> cfe/trunk/unittests/Format/FormatTestSelective.cpp
>
> Modified: cfe/trunk/docs/ClangFormatStyleOptions.rst
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ClangFormatStyleOptions.rst?rev=356031&r1=356030&r2=356031&view=diff
> ==============================================================================
> --- cfe/trunk/docs/ClangFormatStyleOptions.rst (original)
> +++ cfe/trunk/docs/ClangFormatStyleOptions.rst Wed Mar 13 01:26:39 2019
> @@ -365,10 +365,47 @@ the configuration (without a prefix: ``A
> };
> void f() { bar(); }
>
> +**AllowShortIfStatementsOnASingleLine** (``ShortIfStyle``)
> + Dependent on the value, ``if (a) return 0;`` can be put on a
> + single line.
>
> + Possible values:
>
> -**AllowShortIfStatementsOnASingleLine** (``bool``)
> - If ``true``, ``if (a) return;`` can be put on a single line.
> + * ``SIS_Never`` (in configuration: ``Never``)
> + Do not allow short if functions.
> +
> + .. code-block:: c++
> +
> + if (a)
> + return;
> + else
> + return;
> +
> + * ``SIS_WithoutElse`` (in configuration: ``WithoutElse``)
> + Allow short if functions on the same line, as long as else
> + is not a compound statement.
> +
> + .. code-block:: c++
> +
> + if (a) return;
> + else
> + return;
> +
> + if (a)
> + return;
> + else {
> + return;
> + }
> +
> + * ``SIS_Always`` (in configuration: ``Always``)
> + Allow short if statements even if the else is a compound statement.
> +
> + .. code-block:: c++
> +
> + if (a) return;
> + else {
> + return;
> + }
>
> **AllowShortLoopsOnASingleLine** (``bool``)
> If ``true``, ``while (true) continue;`` can be put on a single
>
> Modified: cfe/trunk/include/clang/Format/Format.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Format/Format.h?rev=356031&r1=356030&r2=356031&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Format/Format.h (original)
> +++ cfe/trunk/include/clang/Format/Format.h Wed Mar 13 01:26:39 2019
> @@ -241,8 +241,38 @@ struct FormatStyle {
> /// single line.
> ShortFunctionStyle AllowShortFunctionsOnASingleLine;
>
> + /// Different styles for handling short if lines
> + enum ShortIfStyle {
> + /// Never put short ifs on the same line.
> + /// \code
> + /// if (a)
> + /// return ;
> + /// else {
> + /// return;
> + /// }
> + /// \endcode
> + SIS_Never,
> + /// Without else put short ifs on the same line only if
> + /// the else is not a compound statement.
> + /// \code
> + /// if (a) return;
> + /// else
> + /// return;
> + /// \endcode
> + SIS_WithoutElse,
> + /// Always put short ifs on the same line if
> + /// the else is not a compound statement or not.
> + /// \code
> + /// if (a) return;
> + /// else {
> + /// return;
> + /// }
> + /// \endcode
> + SIS_Always,
> + };
> +
> /// If ``true``, ``if (a) return;`` can be put on a single line.
> - bool AllowShortIfStatementsOnASingleLine;
> + ShortIfStyle AllowShortIfStatementsOnASingleLine;
>
> /// If ``true``, ``while (true) continue;`` can be put on a single
> /// line.
>
> Modified: cfe/trunk/lib/Format/Format.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=356031&r1=356030&r2=356031&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Format/Format.cpp (original)
> +++ cfe/trunk/lib/Format/Format.cpp Wed Mar 13 01:26:39 2019
> @@ -106,6 +106,18 @@ template <> struct ScalarEnumerationTrai
> }
> };
>
> +template <> struct ScalarEnumerationTraits<FormatStyle::ShortIfStyle> {
> + static void enumeration(IO &IO, FormatStyle::ShortIfStyle &Value) {
> + IO.enumCase(Value, "Never", FormatStyle::SIS_Never);
> + IO.enumCase(Value, "Always", FormatStyle::SIS_Always);
> + IO.enumCase(Value, "WithoutElse", FormatStyle::SIS_WithoutElse);
> +
> + // For backward compatibility.
> + IO.enumCase(Value, "false", FormatStyle::SIS_Never);
> + IO.enumCase(Value, "true", FormatStyle::SIS_WithoutElse);
> + }
> +};
> +
> template <> struct ScalarEnumerationTraits<FormatStyle::BinPackStyle> {
> static void enumeration(IO &IO, FormatStyle::BinPackStyle &Value) {
> IO.enumCase(Value, "Auto", FormatStyle::BPS_Auto);
> @@ -631,7 +643,7 @@ FormatStyle getLLVMStyle(FormatStyle::La
> LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
> LLVMStyle.AllowShortBlocksOnASingleLine = false;
> LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
> - LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
> + LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
> LLVMStyle.AllowShortLoopsOnASingleLine = false;
> LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
> LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
> @@ -737,7 +749,8 @@ FormatStyle getGoogleStyle(FormatStyle::
>
> GoogleStyle.AccessModifierOffset = -1;
> GoogleStyle.AlignEscapedNewlines = FormatStyle::ENAS_Left;
> - GoogleStyle.AllowShortIfStatementsOnASingleLine = true;
> + GoogleStyle.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> GoogleStyle.AllowShortLoopsOnASingleLine = true;
> GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
> GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
> @@ -804,7 +817,7 @@ FormatStyle getGoogleStyle(FormatStyle::
> GoogleStyle.AlignOperands = false;
> GoogleStyle.AlignTrailingComments = false;
> GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
> - GoogleStyle.AllowShortIfStatementsOnASingleLine = false;
> + GoogleStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
> GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
> GoogleStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
> GoogleStyle.ColumnLimit = 100;
> @@ -846,7 +859,8 @@ FormatStyle getGoogleStyle(FormatStyle::
> FormatStyle getChromiumStyle(FormatStyle::LanguageKind Language) {
> FormatStyle ChromiumStyle = getGoogleStyle(Language);
> if (Language == FormatStyle::LK_Java) {
> - ChromiumStyle.AllowShortIfStatementsOnASingleLine = true;
> + ChromiumStyle.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
> ChromiumStyle.ContinuationIndentWidth = 8;
> ChromiumStyle.IndentWidth = 4;
> @@ -859,12 +873,12 @@ FormatStyle getChromiumStyle(FormatStyle
> };
> ChromiumStyle.SortIncludes = true;
> } else if (Language == FormatStyle::LK_JavaScript) {
> - ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
> + ChromiumStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
> ChromiumStyle.AllowShortLoopsOnASingleLine = false;
> } else {
> ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
> ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
> - ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
> + ChromiumStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
> ChromiumStyle.AllowShortLoopsOnASingleLine = false;
> ChromiumStyle.BinPackParameters = false;
> ChromiumStyle.DerivePointerAlignment = false;
>
> Modified: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp?rev=356031&r1=356030&r2=356031&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp (original)
> +++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Wed Mar 13 01:26:39 2019
> @@ -413,10 +413,12 @@ private:
> if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,
> TT_LineComment))
> return 0;
> - // Only inline simple if's (no nested if or else).
> - if (I + 2 != E && Line.startsWith(tok::kw_if) &&
> - I[2]->First->is(tok::kw_else))
> - return 0;
> + // Only inline simple if's (no nested if or else), unless specified
> + if (Style.AllowShortIfStatementsOnASingleLine != FormatStyle::SIS_Always) {
> + if (I + 2 != E && Line.startsWith(tok::kw_if) &&
> + I[2]->First->is(tok::kw_else))
> + return 0;
> + }
> return 1;
> }
>
>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=356031&r1=356030&r2=356031&view=diff
> ==============================================================================
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Mar 13 01:26:39 2019
> @@ -439,7 +439,8 @@ TEST_F(FormatTest, FormatIfWithoutCompou
>
> FormatStyle AllowsMergedIf = getLLVMStyle();
> AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
> - AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
> + AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> verifyFormat("if (a)\n"
> " // comment\n"
> " f();",
> @@ -487,6 +488,41 @@ TEST_F(FormatTest, FormatIfWithoutCompou
> verifyFormat("if (a)\n return;", AllowsMergedIf);
> }
>
> +TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
> + FormatStyle AllowsMergedIf = getLLVMStyle();
> + AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
> + AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> + verifyFormat("if (a)\n"
> + " f();\n"
> + "else {\n"
> + " g();\n"
> + "}",
> + AllowsMergedIf);
> + verifyFormat("if (a)\n"
> + " f();\n"
> + "else\n"
> + " g();\n",
> + AllowsMergedIf);
> +
> + AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
> +
> + verifyFormat("if (a) f();\n"
> + "else {\n"
> + " g();\n"
> + "}",
> + AllowsMergedIf);
> + verifyFormat("if (a) f();\n"
> + "else {\n"
> + " if (a) f();\n"
> + " else {\n"
> + " g();\n"
> + " }\n"
> + " g();\n"
> + "}",
> + AllowsMergedIf);
> +}
> +
> TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
> FormatStyle AllowsMergedLoops = getLLVMStyle();
> AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
> @@ -515,7 +551,8 @@ TEST_F(FormatTest, FormatShortBracedStat
> AllowSimpleBracedStatements.ColumnLimit = 40;
> AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
>
> - AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
> + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
>
> AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
> @@ -563,7 +600,8 @@ TEST_F(FormatTest, FormatShortBracedStat
> "};",
> AllowSimpleBracedStatements);
>
> - AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
> + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_Never;
> verifyFormat("if (true) {}", AllowSimpleBracedStatements);
> verifyFormat("if (true) {\n"
> " f();\n"
> @@ -588,7 +626,8 @@ TEST_F(FormatTest, FormatShortBracedStat
> "}",
> AllowSimpleBracedStatements);
>
> - AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
> + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
> AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
>
> @@ -625,7 +664,8 @@ TEST_F(FormatTest, FormatShortBracedStat
> "}",
> AllowSimpleBracedStatements);
>
> - AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
> + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_Never;
> verifyFormat("if (true) {}", AllowSimpleBracedStatements);
> verifyFormat("if (true)\n"
> "{\n"
> @@ -659,7 +699,7 @@ TEST_F(FormatTest, FormatShortBracedStat
> TEST_F(FormatTest, ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
> FormatStyle Style = getLLVMStyleWithColumns(60);
> Style.AllowShortBlocksOnASingleLine = true;
> - Style.AllowShortIfStatementsOnASingleLine = true;
> + Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
> Style.BreakBeforeBraces = FormatStyle::BS_Allman;
> EXPECT_EQ("#define A \\\n"
> " if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
> @@ -3157,7 +3197,7 @@ TEST_F(FormatTest, GraciouslyHandleIncor
>
> TEST_F(FormatTest, FormatsJoinedLinesOnSubsequentRuns) {
> FormatStyle SingleLine = getLLVMStyle();
> - SingleLine.AllowShortIfStatementsOnASingleLine = true;
> + SingleLine.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
> verifyFormat("#if 0\n"
> "#elif 1\n"
> "#endif\n"
> @@ -8000,7 +8040,8 @@ TEST_F(FormatTest, FormatHashIfExpressio
>
> TEST_F(FormatTest, MergeHandlingInTheFaceOfPreprocessorDirectives) {
> FormatStyle AllowsMergedIf = getGoogleStyle();
> - AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
> + AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> verifyFormat("void f() { f(); }\n#error E", AllowsMergedIf);
> verifyFormat("if (true) return 42;\n#error E", AllowsMergedIf);
> verifyFormat("if (true)\n#error E\n return 42;", AllowsMergedIf);
> @@ -10425,7 +10466,8 @@ TEST_F(FormatTest, AllmanBraceBreaking)
> AllmanBraceStyle.ColumnLimit = 80;
>
> FormatStyle BreakBeforeBraceShortIfs = AllmanBraceStyle;
> - BreakBeforeBraceShortIfs.AllowShortIfStatementsOnASingleLine = true;
> + BreakBeforeBraceShortIfs.AllowShortIfStatementsOnASingleLine =
> + FormatStyle::SIS_WithoutElse;
> BreakBeforeBraceShortIfs.AllowShortLoopsOnASingleLine = true;
> verifyFormat("void f(bool b)\n"
> "{\n"
> @@ -10887,7 +10929,6 @@ TEST_F(FormatTest, ParsesConfigurationBo
> CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
> CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
> CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
> - CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
> CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
> CHECK_PARSE_BOOL(BinPackArguments);
> CHECK_PARSE_BOOL(BinPackParameters);
> @@ -11150,6 +11191,20 @@ TEST_F(FormatTest, ParsesConfiguration)
> CHECK_PARSE("NamespaceIndentation: All", NamespaceIndentation,
> FormatStyle::NI_All);
>
> + Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
> + CHECK_PARSE("AllowShortIfStatementsOnASingleLine: Never",
> + AllowShortIfStatementsOnASingleLine, FormatStyle::SIS_Never);
> + CHECK_PARSE("AllowShortIfStatementsOnASingleLine: WithoutElse",
> + AllowShortIfStatementsOnASingleLine,
> + FormatStyle::SIS_WithoutElse);
> + CHECK_PARSE("AllowShortIfStatementsOnASingleLine: Always",
> + AllowShortIfStatementsOnASingleLine, FormatStyle::SIS_Always);
> + CHECK_PARSE("AllowShortIfStatementsOnASingleLine: false",
> + AllowShortIfStatementsOnASingleLine, FormatStyle::SIS_Never);
> + CHECK_PARSE("AllowShortIfStatementsOnASingleLine: true",
> + AllowShortIfStatementsOnASingleLine,
> + FormatStyle::SIS_WithoutElse);
> +
> // FIXME: This is required because parsing a configuration simply overwrites
> // the first N elements of the list instead of resetting it.
> Style.ForEachMacros.clear();
>
> Modified: cfe/trunk/unittests/Format/FormatTestSelective.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestSelective.cpp?rev=356031&r1=356030&r2=356031&view=diff
> ==============================================================================
> --- cfe/trunk/unittests/Format/FormatTestSelective.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTestSelective.cpp Wed Mar 13 01:26:39 2019
> @@ -98,7 +98,7 @@ TEST_F(FormatTestSelective, ReformatsMov
> }
>
> TEST_F(FormatTestSelective, FormatsIfWithoutCompoundStatement) {
> - Style.AllowShortIfStatementsOnASingleLine = true;
> + Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
> EXPECT_EQ("if (a) return;", format("if(a)\nreturn;", 7, 1));
> EXPECT_EQ("if (a) return; // comment",
> format("if(a)\nreturn; // comment", 20, 1));
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list