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

[Haojian Wu via Phabricator via cfe-commits]

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