[PATCH] D115060: [clang-format][NFC] Code Tidies in UnwrappedLineFormatter

[Owen Pan via Phabricator via cfe-commits]

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