[PATCH] D71356: [Tooling/Syntax] Helpers to find spelled tokens touching a location.

[Sam McCall via Phabricator via cfe-commits]

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