owenpan added inline comments.
================ Comment at: clang/lib/Format/TokenAnnotator.cpp:2488-2490 + (NextToken && NextToken->Tok.isAnyIdentifier()) && + (NextToken->getNextNonComment() && + (NextToken->getNextNonComment()->isOneOf( ---------------- dkt01 wrote: > owenpan wrote: > > `NextToken` is guarded against null on line 2488 but not on lines 2489-2490. > If NextToken is null on 2488, 2489-2490 will be short circuited. You are right. The redundant parens threw me off. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:115-117 + SmallVector<TokenAnnotator::ScopeType> &TrackedScopes) : Style(Style), Line(Line), CurrentToken(Line.First), AutoFound(false), + Keywords(Keywords), Scopes(TrackedScopes) { ---------------- Something like the above. (I prefer `Scopes` to `TrackedScopes` to be consistent with the naming of the other parameters.) ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:1195-1198 + // Handle unbalanced braces. + if (!Scopes.empty()) + Scopes.pop_back(); // Lines can start with '}'. ---------------- I don't think it's about unbalanced braces here. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:2474-2475 + auto IsChainedOperatorAmpOrMember = [](const FormatToken *token) { + return token->isOneOf(tok::amp, tok::period, tok::arrow, tok::arrowstar, + tok::periodstar); + }; ---------------- To help simplify the `if` statement below. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:2480 + // reference. + if (Tok.is(tok::amp) && (PrevToken && PrevToken->Tok.isAnyIdentifier()) && + (!PrevToken->getPreviousNonComment() || ---------------- Redundant parens. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:2481-2482 + if (Tok.is(tok::amp) && (PrevToken && PrevToken->Tok.isAnyIdentifier()) && + (!PrevToken->getPreviousNonComment() || + IsChainedOperatorAmpOrMember(PrevToken->getPreviousNonComment())) && + (NextToken && NextToken->Tok.isAnyIdentifier()) && ---------------- The lambda would check that `PrevToken` is nonnull. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:2483-2487 + (NextToken && NextToken->Tok.isAnyIdentifier()) && + (NextToken->getNextNonComment() && + (IsChainedOperatorAmpOrMember(NextToken->getNextNonComment()) || + NextToken->getNextNonComment()->is(tok::semi)))) { + return TT_BinaryOperator; ---------------- Something like that. ================ Comment at: clang/lib/Format/TokenAnnotator.h:36 }; class AnnotatedLine { ---------------- Consider moving `ScopeType` out of `TokenAnnotator` and keeping it consistent with the style of `LineType` above (and replacing `TokenAnnotator::ScopeType` with `ScopeType` everywhere). ================ Comment at: clang/lib/Format/TokenAnnotator.h:184-191 + enum class ScopeType : int8_t { + // Contained in class declaration/definition. + Class, + // Contained within function definition. + Function, + // Contained within other scope block (loop, if/else, etc). + Other, ---------------- See above. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141959/new/ https://reviews.llvm.org/D141959 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits