[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: include/clang/Tooling/Syntax/Tokens.h:206 + /// DECL(a); + /// spelledTokens() returns {"#", "define", "DECL", "(", "name", ")", "eof"}. + /// FIXME: we do not yet store tokens

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: include/clang/Tooling/Syntax/Tokens.h:206 + /// DECL(a); + /// spelledTokens() returns {"#", "define", "DECL", "(", "name", ")", "eof"}. + /// FIXME: we do not yet store tokens

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

nridge added inline comments. Comment at: include/clang/Tooling/Syntax/Tokens.h:206 + /// DECL(a); + /// spelledTokens() returns {"#", "define", "DECL", "(", "name", ")", "eof"}. + /// FIXME: we do not yet store tokens of directives, like #include, #define, ---

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. In D59887#1508991 , @thakis wrote: > Out of interest: The RecursiveASTVisitorTests are part of the ToolingTests > binary while this adds a new binary TokensTest. Can you say why? > > (No change needed, just curious.) The s

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

thakis added a comment. Another comment: The new binary is called TokensTest but is in a directory "Syntax". For consistency with all other unit test binaries, please either rename the binary to SyntaxTests, or rename the directory to "Tokens". (From the patch description, the former seems more

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

thakis added a comment. Out of interest: The RecursiveASTVisitorTests are part of the ToolingTests binary while this adds a new binary TokensTest. Can you say why? (No change needed, just curious.) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https:

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

This revision was automatically updated to reflect the committed changes. Closed by commit rC361148: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library (authored by ibiryukov, committed by ). Changed prior to commit: https://reviews.llvm.org/D59887?vs=200213&id=200260#toc Reposit

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:115 + /// expansion. + llvm::Optional range(const SourceManager &SM) const; + sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > I think this might need

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 200213. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Fix a comment, reformat - Use assertions instead of returning optionals Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:115 + /// expansion. + llvm::Optional range(const SourceManager &SM) const; + ilya-biryukov wrote: > sammccall wrote: > > I think this might need a more explicit name. It's

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:115 + /// expansion. + llvm::Optional range(const SourceManager &SM) const; + sammccall wrote: > I think this might need a more explicit name. It's reasonably obvious t

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall accepted this revision. sammccall added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:49 + +/// A half-open range inside a particular file, the start offset is included and +/// the end offset is excluded from the range. nit:

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 200030. ilya-biryukov added a comment. - Skip annotation tokens, some of them are reported by the preprocessor but we don't want them in the final expanded stream. - Add functions to compute FileRange of tokens, add tests for it. Repository: rG LLVM

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 200019. ilya-biryukov added a comment. - Remove the function that maps tokens to offsets Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/cl

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 198868. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Use bsearch instead of upper_bound Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.or

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 3 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:78 + /// For debugging purposes. + std::string str() const; + sammccall wrote: > ilya-biryukov wrote: > > sammccall wrot

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:62 + SourceLocation location() const { return Location; } + SourceLocation endLocation() const { +return Location.getLocWithOffset(Le

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 198867. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Check invariants on FileRange construction, unify access to all fields Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 198862. ilya-biryukov marked 6 inline comments as done. ilya-biryukov added a comment. - Move filling the gaps at the end of files to a separate function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:130 + OS << llvm::formatv( + "['{0}'_{1}, '{2}'_{3}) => ['{4}'_{5}, '{6}'_{7})\n", + PrintToken(File.SpelledTokens[M.BeginSpelled]), M.BeginSpelled, il

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 198857. ilya-biryukov added a comment. - Move the constuction logic to a separate class, split into multiple methods. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D598

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:130 + OS << llvm::formatv( + "['{0}'_{1}, '{2}'_{3}) => ['{4}'_{5}, '{6}'_{7})\n", + PrintToken(File.SpelledTokens[M.BeginSpelled]), M.BeginSpelled, sa

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:216 +llvm::dbgs() +<< "$[collect-tokens]: " +<< syntax::Token(T).dumpForTests(SourceMgr) << "\n" what's "$[collect

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 197803. ilya-biryukov added a comment. - Get only expanded stream from the preprocessor, recompute mappings and spelled stream separately Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 197279. ilya-biryukov added a comment. - Simplify collection of tokens - Move dumping code to the bottom of the file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D5988

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall accepted this revision. sammccall marked an inline comment as done. sammccall added a comment. This revision is now accepted and ready to land. Rest is details only. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:59 + explicit Token(const clang::Token &T); +

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 196849. ilya-biryukov added a comment. - Update a comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Tooling/Syntax/Tokens.h c

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. @sammccall, could we do another round of review? I think it's perfect now... (Just kidding :-) ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 196847. ilya-biryukov added a comment. - s/llvm::unittest::ValueIs/llvm::ValueIs. - Add tests for empty macro expansions and directives around macro expansions. - Record all gaps between spelled and expanded tokens. - Tweak test dump to distinguish diffe

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:59 + explicit Token(const clang::Token &T); + + tok::TokenKind kind() const { return Kind; } sammccall wrote: > Token should be copyable I think? Sure, they are copyabl

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 196462. ilya-biryukov marked 53 inline comments as done. ilya-biryukov added a comment. - Simplify rawByExpanded by using a helper function. - Add a FIXME to add spelled-to-expanded mapping - s/raw*/spelled* - Split token collector and token buffer tests

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 195425. ilya-biryukov added a comment. - Simplify rawByExpanded by using a helper function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:146 + llvm::Optional> + toOffsetRange(const Token *Begin, const Token *End, +const SourceManager &SM) const; ilya-biryukov wrote: > ilya-biryukov wrote

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 194683. ilya-biryukov added a comment. - Store a source manager in a token buffer Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Too

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. The new version address most of the comments, there are just a few left in the code doing the mapping from Dmitri, I'll look into simplifying and removing the possibly redundant checks. Please take a look at this version, this is very close to the final state. =

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 194682. ilya-biryukov added a comment. - Fix header comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Tooling/Syntax/Tokens.h

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 194680. ilya-biryukov marked 19 inline comments as done. ilya-biryukov added a comment. - Remove a spamy debug tracing output. - Less debug output, move stuff around, more comments. - Add methods that map expanded tokens to raw tokens. - Rename toOffsets

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 193903. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. Herald added a subscriber: mgrang. Changes: - Add multi-file support, record a single expanded stream and per-file-id raw token streams and mappings. - Rename MacroI

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall added a comment. Not sure what the implications of design changes are, so will defer reviewing details of tokencollector (which generally looks good, but is of course highly coupled to lexer/pp) and range mapping (which I suspect could be simplified, but depends heavily on the model).

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:1 +//===- TokenBuffer.h - store tokens of preprocessed files -*- C++ -*-=// +// ilya-biryukov wrote: > sammccall wrote: > > are you sure TokenBuffer is the cent

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. Will address the rest of the comments later, answering a few questions that popped up first. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72 + /// macro or a name, arguments and parentheses of a function-like macro. + llvm::Arra

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

sammccall added a comment. incomplete, haven't reviewed token collector Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72 + /// macro or a name, arguments and parentheses of a function-like macro. + llvm::ArrayRef macroTokens(const TokenBuffer &B) const; + ///

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 192839. ilya-biryukov marked 19 inline comments as done and 2 inline comments as done. ilya-biryukov added a comment. - Rename various things. - Update doc comments. - Search tokens in the tests by spelling, not by kind. - Add more tests. - Fix typos. -

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov added a comment. Still have a few comments to address in `TokenCollector` and wrt to naming. But apart from this revision is ready for another round. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:120 + /// the original source file. The tranformati

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. A few naming alternatives, will update the review after addressing other comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72 + /// macro or a name, arguments and parentheses o

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

gribozavr added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:69 + /// The tokens after preprocessor replacements. + llvm::ArrayRef tokens(const TokenBuffer &B) const; + /// Tokens that appear in the text of the file, i.e. a name of an object-li

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 192661. ilya-biryukov added a comment. - s/macroMacroInvocation/something else... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 Files: clang/include/clang/Too

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov updated this revision to Diff 192660. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Rename macro expansion to macro invocation everywhere - Tweak comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

Eugene.Zelenko added inline comments. Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:323 + PP.getLangOpts(), Tokens); + auto *CB = CBOwner.get(); + ilya-biryukov wrote: > Eugene.Zelenko wrote: > > Return type is

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:323 + PP.getLangOpts(), Tokens); + auto *CB = CBOwner.get(); + Eugene.Zelenko w

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:130 + /// result. + llvm::ArrayRef expansions() const { return Expansions; } + /// Tokens of macro directives and top-level macro e

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

Eugene.Zelenko added inline comments. Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:8 +//===--===// +#include "clang/Tooling/Syntax/TokenBuffer.h" +#include "clang/Basic/Diagnostic.h" Plea

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr. Herald added subscribers: jdoerfert, mgorny. Herald added a project: clang. TokenBuffer stores the list of tokens for a file obtained after preprocessing. This is a base building block for syntax trees, see [1] for the