sammccall accepted this revision. sammccall added inline comments.
================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:49 + +/// A half-open range inside a particular file, the start offset is included and +/// the end offset is excluded from the range. ---------------- nit: character range (just to be totally explicit)? ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:115 + /// expansion. + llvm::Optional<FileRange> range(const SourceManager &SM) const; + ---------------- I think this might need a more explicit name. It's reasonably obvious that this needs to be optional for some cases (token pasting), but it's not obvious at callsites that (or why) we don't use the spelling location for macro-expanded tokens. One option would be just do that and add an expandedFromMacro() helper so the no-macros version is easy to express too. If we can't do that, maybe `directlySpelledRange` or something? ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:121 + /// are from different files or \p Last is located before \p First. + static llvm::Optional<FileRange> range(const SourceManager &SM, + const syntax::Token &First, ---------------- similar to above, I'd naively expect this to return a valid range, given the tokens expanded from `assert(X && [[Y.z()]] )` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
