[PATCH] D59408: [clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present
Krasimir Georgiev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 5 06:25:24 PDT 2019
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
More information about the cfe-commits
mailing list