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

[Krasimir Georgiev via Phabricator via llvm-commits]

krasimir added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:411
   * ``SIS_Always`` (in configuration: ``Always``)
-    Allow short if statements even if the else is a compound statement.
+    Allow short if/else if/else statements when they are not compound statements.
 
----------------
nit: I prefer the term `if statement` instead of `if/else statement`.


================
Comment at: clang/include/clang/Format/Format.h:263
     SIS_WithoutElse,
-    /// Always put short ifs on the same line if
-    /// the else is not a compound statement or not.
+    /// If Else statements have no braces don't put them
+    /// on the same line.
----------------
MyDeveloperDay wrote:
> klimek wrote:
> > This seems weird - why would we want this?
> This is a compatiblity case for the previous "true" as it was before
> 
> Before 
> 
> 
> ```
> if (x) 
>    return 1;
> else
>    return 2;
> ```
> 
> would be transformed to
> 
> 
> ```
> if (x) return 1;
> else
>    return 2;
> ```
> 
> but
> 
> 
> ```
> if (x) 
>    return 1;
> else {
>    return 2;
> }
> ```
> 
> would not be transformed at all
> 
ditto nit: replace `If Else statements` with `If statements`.


================
Comment at: clang/include/clang/Format/Format.h:272
+    /// Always put short if/else/else if on the same line if
+    /// the not compound statement.
     /// \code
----------------
this sentence does not parse. Consider rewriting.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:368
+      return (Style.AllowShortIfStatementsOnASingleLine >=
+              FormatStyle::SIS_AlwaysNoElse)
+                 ? tryMergeSimpleControlStatement(I, E, Limit)
----------------
nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:422
+    if (Line.Last->isNot(tok::r_paren) &&
+        Style.AllowShortIfStatementsOnASingleLine < FormatStyle::SIS_Always)
       return 0;
----------------
nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:430
     // Only inline simple if's (no nested if or else), unless specified
-    if (Style.AllowShortIfStatementsOnASingleLine != FormatStyle::SIS_Always) {
+    if (Style.AllowShortIfStatementsOnASingleLine <
+        FormatStyle::SIS_AlwaysNoElse) {
----------------
nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

Or extract a function that provides this functionality here and in other places.


================
Comment at: clang/unittests/Format/FormatTest.cpp:520
+  verifyFormat("if (a) f();\n"
+               "else if (b) g();\n",
+               AllowsMergedIf);
----------------
I don't understand why is this preferred over:
```
if (a) f();
else
  if (b) g();
```

Isn't syntactically the nested `if (b) g();` a compound statement? I might be wrong.


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

https://reviews.llvm.org/D59408