[PATCH] D67695: [clangd] Implement getBeginning for overloaded operators.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly NITs, except the naming of the new `TokenKind` enum.
I think it's better to pick something that's not clashing with `clang::tok::TokenKind`, even if the enum is in a different namespace.



================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:242
+
+enum TokenKind { Identifier, Operator, Whitespace, Other };
+
----------------
`TokenKind` has the same name as `tok::TokenKind`. Could we use a different name here to avoid possible confusions?
E.g. `TokenGroup` or `TokenFlavor`.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:258
+
+TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
+                       const LangOptions &LangOpts) {
----------------
NIT: add `static` for consistency with the rest of the function.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:261
+  Token Tok;
+  Tok.setKind(tok::TokenKind::NUM_TOKENS);
+  if (Lexer::getRawToken(Loc, Tok, SM, LangOpts,
----------------
NIT: can this just be `tok::NUM_TOKENS`? same for other kinds in the same function


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:311
+
+  // Case 2.
+  if (BeforeTokBeginning == CurrentTokBeginning) {
----------------
NIT: Could you please duplicate what `case 2` is to avoid the need to go back to the comment at the top.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:326
+
+  // Handle case 1 and 3. Note that the cursor could be at the token boundary,
+  // e.g. "Before^Current", we prefer identifiers to other tokens.
----------------
`could be at the token boundary` should be `is at the token boundary`, right?
Whenever it's not the case we'll bail out on `BeforeTokBeginning == CurrentTokBeginning`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67695/new/

https://reviews.llvm.org/D67695