sammccall added inline comments.
================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:320 +const syntax::Token * +spelledIdentifierTouching(SourceLocation Loc, + const syntax::TokenBuffer &Tokens); ---------------- kbobyrev wrote: > Maybe `llvm::Optional<>` here? Pointers are the idiomatic way to express "optional, by-reference" in LLVM. By contrast, optional<Token&> is illegal, and optional<token*> is unidiomatic ================ Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:254 + const syntax::TokenBuffer &Tokens) { + assert(Loc.isFileID()); + llvm::ArrayRef<syntax::Token> All = ---------------- kbobyrev wrote: > Nit: maybe mention this requirement in the header comment? It is mentioned - Loc must be a spelling location. ================ Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:257 + Tokens.spelledTokens(Tokens.sourceManager().getFileID(Loc)); + // SourceLocation < SourceLocation is OK for one file ID. + auto *Right = llvm::partition_point( ---------------- kbobyrev wrote: > Nit: "Comparing SourceLocations within one file using operator less/< is a > valid operation"? Can't come up with something better, but this is slightly > hard to understand. done (I think). FileID rather than file here, because it's not true (or at least not clear) about files. ================ Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:262 + bool AcceptLeft = Right != All.begin() && !((Right - 1)->endLocation() < Loc); + return llvm::makeArrayRef(Right - int(AcceptLeft), Right + int(AcceptRight)); +} ---------------- kbobyrev wrote: > Nit: static casts here or in variable declarations? Replaced with an explicit ternary instead, which is (maybe) clearer? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71356/new/ https://reviews.llvm.org/D71356 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits