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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits