sammccall planned changes to this revision.
sammccall marked 5 inline comments as done.
sammccall added a comment.
Thanks! You caught a serious bug, I'm digging into it.
================
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() ||
----------------
hokein wrote:
> if it is empty, I think it is more appropriate to return `NoTokens` (the old
> behavior)
Whoops, this was meant to be this->ExpandedTokens.
You're right that in principle ExpandedTokens.empty() should return NoTokens.
(We would never actually get here).
Similarly in principle !ExpandedTokens.empty() && SpelledTokens.empty() should
return Unselected instead of NoTokens. This could happen but didn't make any
difference to observable behavior.
I've fixed all these in any case.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:290
+ if (ExpandedTokens.empty() ||
+ &ExpandedTokens.front() > &this->ExpandedTokens.back() ||
+ &ExpandedTokens.back() < &this->ExpandedTokens.front()) {
----------------
hokein wrote:
> 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
>
>
ExpandedTokens -> MaybeSelectedExpanded
SpelledTokens -> SelectedSpelled
================
Comment at: clang-tools-extra/clangd/Selection.cpp:350
+ return *Offset < First;
+ StartInvalid = false;
+ return false;
----------------
hokein wrote:
> this should be `true`.
Oh shoot, now the assert is firing, I must have missed some cases :-(
Need to fix this.
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
