From 6d294f28e9a24ca941f88bce7c8ed98d5931a3fc Mon Sep 17 00:00:00 2001 From: Paul Hoad Date: Wed, 13 Mar 2019 08:07:46 +0000 Subject: [PATCH] [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 llvm-svn: 356029 --- .../checks/readability-identifier-naming.rst | 2 +- clang-tools-extra/docs/conf.py | 2 +- clang/docs/ClangFormatStyleOptions.rst | 41 +++++++++- clang/include/clang/Format/Format.h | 32 +++++++- clang/lib/Format/Format.cpp | 26 +++++-- clang/lib/Format/UnwrappedLineFormatter.cpp | 10 ++- clang/unittests/Format/FormatTest.cpp | 75 ++++++++++++++++--- .../unittests/Format/FormatTestSelective.cpp | 2 +- llvm/utils/release/build_llvm_package.bat | 14 ++-- 9 files changed, 171 insertions(+), 33 deletions(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst index d7e885935618..83546c6c3da8 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst @@ -9,7 +9,7 @@ This check will try to enforce coding guidelines on the identifiers naming. It supports one of the following casing types and tries to convert from one to another if a mismatch is detected -Casing types inclde: +Casing types include: - ``lower_case``, - ``UPPER_CASE``, diff --git a/clang-tools-extra/docs/conf.py b/clang-tools-extra/docs/conf.py index 16e81051f753..3741fdf544fc 100644 --- a/clang-tools-extra/docs/conf.py +++ b/clang-tools-extra/docs/conf.py @@ -26,7 +26,7 @@ from datetime import date # Add any Sphinx extension module names here, as strings. They can be extensions # coming with Sphinx (named 'sphinx.ext.*') or your custom ones. -extensions = ['sphinx.ext.todo', 'sphinx.ext.mathjax'] +extensions = ['sphinx.ext.todo', 'sphinx.ext.mathjax', 'sphinxcontrib.spelling' ] # Add any paths that contain templates here, relative to this directory. templates_path = ['_templates'] diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst index 582b9d938cc1..d4a56d3bb3cc 100644 --- a/clang/docs/ClangFormatStyleOptions.rst +++ b/clang/docs/ClangFormatStyleOptions.rst @@ -365,10 +365,47 @@ the configuration (without a prefix: ``Auto``). }; 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 diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h index 94cceb30dcaa..4d34a559c8e6 100644 --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -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. diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp index a56fed9dee1e..ce67b82e10bd 100644 --- a/clang/lib/Format/Format.cpp +++ b/clang/lib/Format/Format.cpp @@ -106,6 +106,18 @@ template <> struct ScalarEnumerationTraits { } }; +template <> struct ScalarEnumerationTraits { + 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 { static void enumeration(IO &IO, FormatStyle::BinPackStyle &Value) { IO.enumCase(Value, "Auto", FormatStyle::BPS_Auto); @@ -631,7 +643,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) { 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::LanguageKind Language) { 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::LanguageKind Language) { 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::LanguageKind Language) { 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::LanguageKind Language) { }; 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; diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index a62755840df1..f14e3851c8de 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -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; } diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index 650636b76339..7fcff46f581f 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -439,7 +439,8 @@ TEST_F(FormatTest, FormatIfWithoutCompoundStatement) { 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, FormatIfWithoutCompoundStatement) { 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, FormatShortBracedStatements) { 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, FormatShortBracedStatements) { "};", 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, FormatShortBracedStatements) { "}", AllowSimpleBracedStatements); - AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true; + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = + FormatStyle::SIS_WithoutElse; AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true; AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true; @@ -625,7 +664,8 @@ TEST_F(FormatTest, FormatShortBracedStatements) { "}", 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, FormatShortBracedStatements) { 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, GraciouslyHandleIncorrectPreprocessorConditions) { 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, FormatHashIfExpressions) { 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, ParsesConfigurationBools) { 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(); diff --git a/clang/unittests/Format/FormatTestSelective.cpp b/clang/unittests/Format/FormatTestSelective.cpp index b89c04755fd5..f031a3dee54a 100644 --- a/clang/unittests/Format/FormatTestSelective.cpp +++ b/clang/unittests/Format/FormatTestSelective.cpp @@ -98,7 +98,7 @@ TEST_F(FormatTestSelective, ReformatsMovedLines) { } 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)); diff --git a/llvm/utils/release/build_llvm_package.bat b/llvm/utils/release/build_llvm_package.bat index 8038d798aa0a..609bb8d990c6 100755 --- a/llvm/utils/release/build_llvm_package.bat +++ b/llvm/utils/release/build_llvm_package.bat @@ -42,13 +42,13 @@ mkdir %build_dir% cd %build_dir% echo Checking out %branch% at r%revision%... -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/llvm/%branch% llvm || exit /b -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/cfe/%branch% llvm/tools/clang || exit /b -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/clang-tools-extra/%branch% llvm/tools/clang/tools/extra || exit /b -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/lld/%branch% llvm/tools/lld || exit /b -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/compiler-rt/%branch% llvm/projects/compiler-rt || exit /b -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/openmp/%branch% llvm/projects/openmp || exit /b -svn.exe export -r %revision% http://llvm.org/svn/llvm-project/lldb/%branch% llvm/tools/lldb || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/llvm/%branch% llvm || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/cfe/%branch% llvm/tools/clang || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/clang-tools-extra/%branch% llvm/tools/clang/tools/extra || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/lld/%branch% llvm/tools/lld || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/compiler-rt/%branch% llvm/projects/compiler-rt || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/openmp/%branch% llvm/projects/openmp || exit /b +REM svn.exe export -r %revision% http://llvm.org/svn/llvm-project/lldb/%branch% llvm/tools/lldb || exit /b REM Setting CMAKE_CL_SHOWINCLUDES_PREFIX to work around PR27226.