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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
