owenpan added inline comments.
================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:215 + const auto &NextLine = *I[1]; + const auto *PreviousLine = I != AnnotatedLines.begin() ? I[-1] : nullptr; + if (NextLine.Type == LT_Invalid || NextLine.First->MustBreakBefore) ---------------- I would move this line to just before handling empty record blocks below. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:268-289 + [this, B = AnnotatedLines.begin(), &NextLine, + TheLine](SmallVectorImpl<AnnotatedLine *>::const_iterator I) { if (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All) return true; if (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty && + NextLine.First->is(tok::r_brace)) ---------------- I'd either leave this lambda alone or further simplify it as suggested here. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:318 bool MergeShortFunctions = ShouldMergeShortFunctions(I); ---------------- Drop the `I` if you decide to further simplify the lambda as suggested above. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:406-417 + if (PreviousLine && TheLine->First->is(tok::l_brace) && + PreviousLine->First->is(tok::at) && PreviousLine->First->Next) { + tok::ObjCKeywordKind kwId = + PreviousLine->First->Next->Tok.getObjCKeywordID(); if (kwId == clang::tok::objc_autoreleasepool || kwId == clang::tok::objc_synchronized) return 0; ---------------- If you want, you can factor out `PreviousLine && TheLine->First->is(tok::l_brace)` and even combine the two `if` statements as they both return 0. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:310 + : 0; + } + if (TheLine->First->isOneOf(tok::kw_if, tok::kw_else, tok::kw_while, ---------------- owenpan wrote: > I don't think you can drop `else` here. > I don't think you can drop `else` here. I was wrong. For some reason, I didn't see the `return` statement. ================ Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:317 + : 0; + } + if (TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) && ---------------- HazardyKnusperkeks wrote: > owenpan wrote: > > Ditto. > Same. I missed the `return` here as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115060/new/ https://reviews.llvm.org/D115060 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits