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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
