[PATCH] D141959: [clang-format] Fix inconsistent identification of operator&

[Owen Pan via Phabricator via cfe-commits]

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