[PATCH] D67358: [clangd] Implement semantic selections.

[Sam McCall via Phabricator via cfe-commits]

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

Nice!

Please make sure you run clang-format (I think it'll add missing newlines at EOF)



================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:22
+// Assumes that only consecutive ranges can coincide.
+void addIfDistinct(const Range &R, std::vector<Range> *Result) {
+  if (Result->empty() || Result->back() != R) {
----------------
nit: it's idiomatic in LLVM to pass mutable references rather than pointers where possible


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52
+    auto SR = toHalfOpenFileRange(SM, LangOpts, Node->ASTNode.getSourceRange());
+    if (!SR.hasValue()) {
+      continue;
----------------
`|| SM.getFileID(SR->getBegin()) != SM.getMainFileID()`

(begin and end are guaranteed to be the same file, but it may not be the main file)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.h:22
+
+// Returns the list of all interesting ranges around the Position \p Pos.
+// The interesting ranges corresponds to the AST nodes in the SelectionTree
----------------
nit: if you intend to use doxygen comments (with `\p` syntax) you probably want `///` so doxygen will render them


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358