kbobyrev marked an inline comment as done.
kbobyrev added inline comments.
================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:320
+const syntax::Token *
+spelledIdentifierTouching(SourceLocation Loc,
+ const syntax::TokenBuffer &Tokens);
----------------
sammccall wrote:
> 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
Ah, I see, I thought a copy is quite cheap in this case. Makes sense then.
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:254
+ const syntax::TokenBuffer &Tokens) {
+ assert(Loc.isFileID());
+ llvm::ArrayRef<syntax::Token> All =
----------------
sammccall wrote:
> kbobyrev wrote:
> > Nit: maybe mention this requirement in the header comment?
> It is mentioned - Loc must be a spelling location.
Ah, okay, I didn't know that spelling location implies exactly this property,
my bad.
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
