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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good.



================
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.
----------------
sorry, I don't quite understand this sentence...


================
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
----------------
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.


================
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)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116978