[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:
> 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.
Ah, it is, so Optional<Token> would be an option. But the address is meaningful - spelled tokens have contiguous storage that indicates their sequence. So returning a pointer is actually more useful.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:261
+  bool AcceptRight = Right != All.end() && !(Loc < Right->location());
+  bool AcceptLeft = Right != All.begin() && !((Right - 1)->endLocation() < Loc);
+  return llvm::makeArrayRef(Right - (AcceptLeft ? 1 : 0),
----------------
ilya-biryukov wrote:
> Maybe compare offsets instead of locations?
> It might be more code, but it would read much cleaner.
> 
> It always feels weird to compare source locations for anything other than storing them in `std::map`
I find the `SM.getFileOffset(...)` everywhere pretty hard to read and obscures the actual locations.
But agree about the operators, I've added the rest. Having `operator<` is pretty evil, but I don't think the others are particularly incrementally evil.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:270
+  for (const syntax::Token &Tok : spelledTokensTouching(Loc, Tokens))
+    if (Tok.kind() == tok::identifier)
+      return &Tok;
----------------
ilya-biryukov wrote:
> NIT: add braces around `if` statement
Is there some reference for preferred LLVM style for this, or personal preference? (Real question, as this comes up a bunch)

I ask because that's my *least*-preferred option - no braces > braces on for > braces on both > braces on (only) if.

Added braces to the `for` (maybe that's what you meant?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71356/new/

https://reviews.llvm.org/D71356