[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:62
+  SourceLocation location() const { return Location; }
+  SourceLocation endLocation() const {
+    return Location.getLocWithOffset(Length);
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > sadly, this would need documentation - it's the one-past-the-end location, not the last character in the token, not the location of the "next" token, or the location of the "next" character...
> > > 
> > > I do wonder whether this is actually the right function to expose...
> > >  Do you ever care about end but not start? (The reverse seems likelier). Having two independent source location accessors obscures the invariant that they have the same file ID.
> > > 
> > > I think exposing a `FileRange` accessor instead might be better, but for now you could also make callers use `Tok.location().getLocWithOffset(Tok.length())` until we know it's the right pattern to encourage
> > Added a comment. With the comment and an implementation being inline and trivial, I don't think anyone would have trouble understanding what this method does.
> (this is ok. I do think a FileRange accessor would make client code more readable/less error-prone. Let's discuss offline)
I've added a corresponding accessor (and a version of it that accepts a range of tokens) to D61681.
I'd keep it off this patch for now, will refactor in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59887