Hahnfeld added inline comments.
================
Comment at: clang/lib/Lex/Preprocessor.cpp:1000
+ std::vector<Token> toks;
+ while (1) {
+ Token tok;
----------------
v.g.vassilev wrote:
> Hahnfeld wrote:
> > aaron.ballman wrote:
> > > v.g.vassilev wrote:
> > > > aaron.ballman wrote:
> > > > > Hahnfeld wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I'd prefer not to assume the token stream has an EOF token
> > > > > > > (perhaps the stream is one only being used to parse until the
> > > > > > > `eod` token instead), so if we can turn this into a non-infinite
> > > > > > > loop, that would make me more comfortable.
> > > > > > I'm not sure I understand entirely. Do you want something like
> > > > > > ```
> > > > > > tok.isOneOf(tok::unknown, tok::eof, tok::eod)
> > > > > > ```
> > > > > > instead of `tok.is(tok::eof)`? Can this happen at the level of the
> > > > > > `Preprocessor`?
> > > > > I was thinking something more along the lines of:
> > > > > ```
> > > > > if (Tokens) {
> > > > > for (Token Tok; !Tok.isOneOf(tok::eof, tok::eod); Lex(Tok))
> > > > > Tokens->push_back(Tok);
> > > > > }
> > > > > ```
> > > > > but I hadn't thought about `tok::unknown`; that might be a good one
> > > > > to also include given that clangd operates on partial sources.
> > > > >
> > > > I was wondering if we could somehow merge this routine with
> > > > `Parser::SkipUntil` since they seem to be doing a very similar tasks.
> > > That could perhaps end up looking reasonable (they do similar tasks aside
> > > from collecting the tokens that are being skipped). Do you need the
> > > interface to be on `Preprocessor` or `Parser` though (or does it not
> > > really matter for you)?
> > > `tok.isOneOf(tok::unknown, tok::eof, tok::eod)`
> >
> > I implemented this check, let me know if this looks reasonable. The code
> > you posted doesn't do what we need because we also want to lex if `Tokens`
> > is `nullptr`, so the hierarchy must be an `if` inside the loop.
> > Given these additional token kinds, does `UntilEOF` still make sense or do
> > we want another name? Note that I'll leave `repl_input_end` to
> > https://reviews.llvm.org/D158415.
> >
> > > I was wondering if we could somehow merge this routine with
> > > `Parser::SkipUntil` since they seem to be doing a very similar tasks.
> >
> > I'm not sure this makes sense, given that `Parser::SkipUntil` requires some
> > knowledge about the structural input. At the very least, I'd prefer not to
> > go into that direction for this change.
> I am not sure I understand the reasoning but I somewhat see that having
> Parser's `SkipUntil` be implemented with our new `Preprocessor::LexUntil...`
> would require a lot more work. How about adding a fixme note capturing this
> as a future possible refactoring?
@v.g.vassilev `Parser::SkipUntil` has a long `switch` statement looking at the
token and taking special actions depending on their kind and the context it
appears in. I don't see how to generalize this in the way `LexTokensUntilEOF`
works.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158413/new/
https://reviews.llvm.org/D158413
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
