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