[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

This looks much better now, thanks for taking another look! I've flagged some formatting/spelling nits, and I think the tests/docs could be a little more explicit about the behaviour of `WithoutElse`. Other than that, looks great!



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:375
+  * ``SIS_Never`` (in configuration: ``Never``)
+    Do not allow short if functions
+
----------------
For consistency, these descriptions should end with periods `.`


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:386
+    Allow short if functions on the same line, as long as else
+    is not a compound statement
 
----------------
Missing `.`


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:390
+
+       if (a) return;
+       else 
----------------
I think an example of the generated formatting *with* a compound else statement would be useful here too, in addition to the existing example.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:395
+  * ``SIS_Always`` (in configuration: ``Always``)
+    Allow short if statements even if the else is a compounf statement
+
----------------
compounf -> compound


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:395
+  * ``SIS_Always`` (in configuration: ``Always``)
+    Allow short if statements even if the else is a compounf statement
+
----------------
reuk wrote:
> compounf -> compound
Missing `.`


================
Comment at: clang/include/clang/Format/Format.h:264
+    /// Always put short ifs on the same line if
+    /// the else is not a compound statement or not
+    /// \code
----------------
Missing `.`


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:417
+    // 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) &&
----------------
Has this line been clang-formatted? I'd expect spaces around the binop


================
Comment at: clang/unittests/Format/FormatTest.cpp:494
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
+  verifyFormat("if (a)\n"
+               "  f();\n"
----------------
I think having a test here which shows the expected behaviour for non-compound else statements with `WithoutElse` would be helpful, as in the documentation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59087/new/

https://reviews.llvm.org/D59087