[PATCH] D116978: [clangd] Selection: Prune gtest TEST()s earlier

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:306
+    // If the node starts after the selection ends, it is not selected.
+    // All tokens a macro location might claim are >= its expansion site.
+    // So it's safe to use the expansions location for the comparison.
----------------
hokein wrote:
> sorry, I don't quite understand this sentence...
Rephrased.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:315
+    while (EndLoc.isMacroID())
+      EndLoc = SM.getImmediateExpansionRange(EndLoc).getEnd();
+    // In the rare case that the expansion range is a char range, EndLoc is
----------------
hokein wrote:
> I think the current implementation is fine, just notice there is an alternative
> -- SourceManager::getExpansionRange(SourceRange) method does similar things, we could use that to save some bits of code, but we will have some extra cost for unnecessary getImmediateExpansionRange/getFileID.
Yeah, it does a fair amount of unneccesary work in order for the endpoint to be exact (handling whether intermediate exp ranges for endpoint are char ranges or token ranges).
For our case we don't care if we're a token too far to the right, and we don't want to pay for all this.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:318
+    // ~one token too far to the right. We may fail to prune, that's OK.
+    if (auto E = offsetInSelFile(EndLoc))
+      if (*E < SpelledTokens.front().Offset)
----------------
hokein wrote:
> as discussed offline, `offsetInSelFile` is doing smart and efficient thing which relies on the underlying offset implementation in source manger, would be nice to have some comments and docs there and SourceManager comparison operator.
Oops, I forgot to document that function! Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116978/new/

https://reviews.llvm.org/D116978