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