hokein added a comment.
this looks good in general.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:289
+ // surround the selection, including when generated by macros.
+ if (ExpandedTokens.empty() ||
+ &ExpandedTokens.front() > &this->ExpandedTokens.back() ||
----------------
if it is empty, I think it is more appropriate to return `NoTokens` (the old
behavior)
================
Comment at: clang-tools-extra/clangd/Selection.cpp:290
+ if (ExpandedTokens.empty() ||
+ &ExpandedTokens.front() > &this->ExpandedTokens.back() ||
+ &ExpandedTokens.back() < &this->ExpandedTokens.front()) {
----------------
I would suggest rename the member `ExpandedTokens`, and `SpellingTokens` to
something like `AffectedSpelledTokens`, `AffectedExpandedTokens`, I think it is
important to express "these are affected-tokens by the selection" in their name
================
Comment at: clang-tools-extra/clangd/Selection.cpp:350
+ return *Offset < First;
+ StartInvalid = false;
+ return false;
----------------
this should be `true`.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:373
+ return *Offset <= Last;
+ EndInvalid = false;
+ return true;
----------------
the same, true.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:378
+ assert(false && "Expanded tokens could not be resolved to main file!");
+ Start = Toks.expandedTokens().end();
+ }
----------------
should be `End`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117107/new/
https://reviews.llvm.org/D117107
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
