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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added inline comments.


================
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:
> sammccall wrote:
> > 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.
> I find it to be the only way to write code, which unambiguously says what it wants to do with source locations.
> 
> Not saying it's pretty, but I the source location API was designed to be used this way. Arguably, using something else in special corner cases does more harm than good (hard-to-read, non-conventional code).
> 
> But up to you.
> But up to you.

I would rather use the operators, with the comment for the unusual usage, if that's OK.

(I agree it doesn't generalize and about the intent of the APIs, but think the APIs hurt readability to the extent that there's less value in consistency than in clear intent at individual callsites).


================
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:
> kbobyrev wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > 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?)
> > > Not sure if it's in LLVM style guide, but files inside Syntax folder definitely use this style: put braces everywhere until you reach the last level of nesting for non-leaf statements (i.e. having other statements as children, e.g. loops,. if statements, etc)
> > > 
> > > 
> > > It's my personal preference, happy to discuss whether having this makes sense.
> > I guess it's a personal preference (also for me), but I don't think there is a strict guideline on that. Interestingly enough, I think there is a piece of code in the styleguide that looks exactly like the code you had: https://llvm.org/docs/CodingStandards.html#turn-predicate-loops-into-predicate-functions
> > 
> > Some Clang subprojects tend to put braces everywhere though.
> > 
> > That being said, I guess no braces at all would be the best option here.
> Yeah, so LLVM style is unclear, but has examples with no braces.
> Could be the argument to get rid of those.
> 
> I'd still stick to my personal preferences where I can, but if everyone thinks no braces is better, happy to go with this option.
I think we're good here - I'm fine with the braces on `for`, and it sounds like that's what you wanted.
(I guess I misread "braces around if statement" as "braces around if body")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71356