[PATCH] D117107: [clangd] Elide even more checks in SelectionTree.

[Sam McCall via Phabricator via cfe-commits]

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