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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


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


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:258
+
+TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
+                       const LangOptions &LangOpts) {
----------------
ilya-biryukov wrote:
> NIT: add `static` for consistency with the rest of the function.
I think the static is redundant here, as the function is in the anonymous namespace, I removed the `static` on the function above.


================
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.
----------------
ilya-biryukov wrote:
> `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`.
yes, you are right. I was thinking of the single token case `int ^foo;`, which should be counted into the boundary case as we consider whitespace as a token kind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67695