[PATCH] D64329: [Clangd] Fixed SelectionTree bug for macros

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:247
       return SelectionTree::Unselected;
-    // getTopMacroCallerLoc() allows selection of constructs in macro args. e.g:
+    // getFileRange() allows selecting macro arg expansions
     //   #define LOOP_FOREVER(Body) for(;;) { Body }
----------------
the new wording seems less clear to me


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:447
   BeginNamespace, // namespace <ns> {.     Payload is resolved <ns>.
-  EndNamespace,   // } // namespace <ns>.  Payload is resolved *outer* namespace.
-  UsingDirective  // using namespace <ns>. Payload is unresolved <ns>.
----------------
It looks like your editor might be set up to format the whole file, rather than just changed lines (`git clang-format` will also do this).

Please avoid this in general as it messes up blame history.
No need to go back and revert everything though.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:655
+// Returns the location of the end of the token present at a given location
+static SourceLocation getLocationAtTokenEnd(SourceLocation Loc,
+                                            const SourceManager &SM,
----------------
This is Lexer::getLocForEndOfToken(Loc, 0,  SM, LangOpts), I think


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:663
+// Finds the union of two ranges. If any of the ranges is a Token Range, it uses
+// the token ending.
+static CharSourceRange unionCharSourceRange(CharSourceRange R1,
----------------
Returns a half-open CharSourceRange.
(Because closed character CharSourceRanges are also a thing...)


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:683
+    return {{Loc, getLocationAtTokenEnd(Loc, SM, LangOpts)}, false};
+  CharSourceRange FileRange({Loc}, false);
+  do {
----------------
nit: do the conversion from the previous line here, and write the loop as a regular while loop?


================
Comment at: clang-tools-extra/clangd/SourceCode.h:204
 
+// Return the FileRange for a given range where the ends can be in different
+// files. Note that the end of the FileRange is the end of the last token.
----------------
This is closely related to `toHalfOpenFileRange` and should be moved up there.

Actually... is this just a less-buggy replacement for `toHalfOpenFileRange`? If so, we should give it that name. It'd be nice to add a few unit tests (and verify that they fail with the old version of the function).


================
Comment at: clang-tools-extra/clangd/SourceCode.h:205
+// Return the FileRange for a given range where the ends can be in different
+// files. Note that the end of the FileRange is the end of the last token.
+SourceRange getFileRange(SourceRange Range, const SourceManager &SM, const LangOptions &LangOpts);
----------------
"end of the last token" is confusing: does it point to the last char, or one-past-last?

The latter seems nicest and is easily described by the existing "half-open" name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64329