MyDeveloperDay added a comment. sorry to be critical, just trying to help get traction on your review...
================ Comment at: lib/Format/TokenAnnotator.cpp:65 - const FormatToken &Previous = *CurrentToken->Previous; // The '<'. + const FormatToken &Previous = *CurrentToken->Previous; // The '<'. if (Previous.Previous) { ---------------- unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass ================ Comment at: lib/Format/TokenAnnotator.cpp:526 if (!Contexts.back().FirstObjCSelectorName) { - FormatToken* Previous = CurrentToken->getPreviousNonComment(); - if (Previous && Previous->is(TT_SelectorName)) { - Previous->ObjCSelectorNameParts = 1; - Contexts.back().FirstObjCSelectorName = Previous; - } + FormatToken *Previous = CurrentToken->getPreviousNonComment(); + if (Previous && Previous->is(TT_SelectorName)) { ---------------- unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass ================ Comment at: lib/Format/TokenAnnotator.cpp:1392 + // FIXME(bug 36976): ObjC return types shouldn't use + // TT_CastRParen. Current.Previous && Current.Previous->is(TT_CastRParen) && ---------------- unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass ================ Comment at: lib/Format/TokenAnnotator.cpp:2420 + Right.MatchingParen->Next && + Right.MatchingParen->Next->is(tok::colon); return !IsLightweightGeneric && Style.ObjCSpaceBeforeProtocolList; ---------------- unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass ================ Comment at: lib/Format/TokenAnnotator.cpp:2545 + Left.Tok.getIdentifierInfo()->isKeyword( + getFormattingLangOpts(Style)))) && + Line.Type != LT_PreprocessorDirective) || ---------------- would be nice to pull the 3 items into a function because you repeat it on line 2551 ================ Comment at: lib/Format/TokenAnnotator.cpp:2548 + (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && + Right.ParameterCount >= 1 && + (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() || ---------------- something similar here when combined with line 2540, looks a lot like the 2 patterns below? almost like its: ``` bool allowSpaceBeforeIfFunctionHasParameters(Style,Right); ``` ================ Comment at: lib/Format/TokenAnnotator.cpp:2617 + if (Left.MatchingParen && + Left.MatchingParen->is(TT_ProtoExtensionLSquare) && Right.isOneOf(tok::l_brace, tok::less)) ---------------- unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass ================ Comment at: lib/Format/TokenAnnotator.cpp:2833 + (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && + Right.ParameterCount >= 1); if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) && ---------------- can this and the above section on line 2745 be combined? I'm not sure if the order of the "if" indicates precedence ================ Comment at: lib/Format/TokenAnnotator.cpp:3138 if (Right.is(Keywords.kw_is)) { - const FormatToken* Next = Right.getNextNonComment(); + const FormatToken *Next = Right.getNextNonComment(); // If `is` is followed by a colon, it's likely that it's a dict key, so ---------------- unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55170 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits