owenpan added inline comments.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:116-117
+ SmallVector<TokenAnnotator::ScopeType> &TrackedScopes)
+ : Scopes(TrackedScopes), Style(Style), Line(Line),
+ CurrentToken(Line.First), AutoFound(false), Keywords(Keywords) {
Contexts.push_back(Context(tok::unknown, 1, /*IsExpression=*/false));
----------------
Please keep them in the same order as that of the parameters above.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:852
+ assert(Scopes.size() > 1);
+ Scopes.pop_back();
assert(OpeningBrace.Optional == CurrentToken->Optional);
----------------
Should we assert that the token type of `OpeningBrace` matches the `ScopeType`
before popping?
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:1162-1169
+ case TT_EnumLBrace:
+ case TT_ControlStatementLBrace:
+ case TT_ElseLBrace:
+ case TT_BracedListLBrace:
+ case TT_CompoundRequirementLBrace:
+ case TT_ObjCBlockLBrace:
+ case TT_RecordLBrace:
----------------
Do we really need these? If we add other special `l_brace` token types, we will
have to update this list.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:1207
+ }
// Lines can start with '}'.
if (Tok->Previous)
----------------
Can you leave the comment at the top of the `case`? If we get rid of `None` for
`ScopeType`, we don't need to pop here. (See below.)
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2485-2487
+ PrevToken->getPreviousNonComment()->isOneOf(tok::amp, tok::period,
+ tok::arrow,
tok::arrowstar,
+ tok::periodstar)) &&
----------------
Can you add a lambda for this so that it can be used below?
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2488-2490
+ (NextToken && NextToken->Tok.isAnyIdentifier()) &&
+ (NextToken->getNextNonComment() &&
+ (NextToken->getNextNonComment()->isOneOf(
----------------
`NextToken` is guarded against null on line 2488 but not on lines 2489-2490.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2491-2492
+ (NextToken->getNextNonComment()->isOneOf(
+ tok::amp, tok::semi, tok::period, tok::arrow, tok::arrowstar,
+ tok::periodstar)))) {
+ return TT_BinaryOperator;
----------------
Here we can use the lambda suggested above.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2778
+ : Style(Style), Keywords(Keywords) {
+ Scopes.push_back(ScopeType::None);
+}
----------------
Instead of pushing `None`, would it better to leave `Scopes` empty? Then we
would not need `None` for the `ScopeType`, and instead of checking for
`Scopes.size() > 1`, we could check if it's empty().
================
Comment at: clang/lib/Format/TokenAnnotator.h:183
+ enum class ScopeType : std::int8_t {
+ // Not contained within scope block.
----------------
Drop `std::`.
================
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:192-194
+ Tokens = annotate("val1 & val2.member;");
+ ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+ EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
----------------
Can you add a case for `periodstar`?
================
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:207-213
+ Tokens =
+ annotate("val1 & val2.member & val3.member() & val4 & val5->member;");
+ ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+ EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+ EXPECT_TOKEN(Tokens[5], tok::amp, TT_BinaryOperator);
+ EXPECT_TOKEN(Tokens[11], tok::amp, TT_BinaryOperator);
+ EXPECT_TOKEN(Tokens[13], tok::amp, TT_BinaryOperator);
----------------
Can you add a case for `arrowstar`?
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
