MyDeveloperDay updated this revision to Diff 190053.
MyDeveloperDay marked 8 inline comments as done.
MyDeveloperDay added a comment.
Address review comment
- add document and comment spelling and punctuation
- add additional unit tests to show non compound else clause case
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59087/new/
https://reviews.llvm.org/D59087
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/FormatTestSelective.cpp
Index: clang/unittests/Format/FormatTestSelective.cpp
===================================================================
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -98,7 +98,7 @@
}
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));
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -439,7 +439,7 @@
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 +487,41 @@
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 +550,7 @@
AllowSimpleBracedStatements.ColumnLimit = 40;
AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
- AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+ AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
@@ -563,7 +598,7 @@
"};",
AllowSimpleBracedStatements);
- AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+ AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
verifyFormat("if (true) {}", AllowSimpleBracedStatements);
verifyFormat("if (true) {\n"
" f();\n"
@@ -588,7 +623,7 @@
"}",
AllowSimpleBracedStatements);
- AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+ AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
@@ -625,7 +660,7 @@
"}",
AllowSimpleBracedStatements);
- AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+ AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
verifyFormat("if (true) {}", AllowSimpleBracedStatements);
verifyFormat("if (true)\n"
"{\n"
@@ -659,7 +694,7 @@
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 +3192,7 @@
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 +8035,7 @@
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 +10460,7 @@
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 +10922,6 @@
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 +11184,18 @@
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();
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -413,10 +413,12 @@
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;
}
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -106,6 +106,18 @@
}
};
+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 @@
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,7 @@
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 +816,7 @@
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 +858,7 @@
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 +871,12 @@
};
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;
Index: clang/include/clang/Format/Format.h
===================================================================
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -241,8 +241,38 @@
/// 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.
Index: clang/docs/ClangFormatStyleOptions.rst
===================================================================
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -365,10 +365,47 @@
};
void f() { bar(); }
+**AllowShortIfStatementsOnASingleLine** (``ShortIfStyle``)
+ Dependent on the value, ``if (a) return 0;`` can be put on a
+ single line.
+
+ Possible values:
+
+ * ``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;
+ }
-**AllowShortIfStatementsOnASingleLine** (``bool``)
- If ``true``, ``if (a) return;`` can be put on a single line.
+ * ``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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
