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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
