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@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@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
