[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: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