[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

tahonermann added a comment. The changes made (from what I've seen, I haven't reviewed every line) make sense to me. The amount of change does make me a bit nervous though. In D143142#4142212 , @aaron.ballman wrote: > In terms of whether to use `unsign

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

aaron.ballman added a comment. In general, I think this makes sense. However: > This change solves this issue nicely. Since we will be only adding code at > the back of the buffer, the offsets are always constant even if we grow the > buffer many times and all the access to new buffer will be v

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

davrec added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3630-3632 do { - ++CurPtr; -} while (isHorizontalWhitespace(*CurPtr)); + ++CurOffset; +} while (isHorizontalWhitespace(BufferStart[CurOffset])); davrec wrote: > ``` > for (is

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

davrec added a comment. Suggested a few adjustments in `LexTokenInternal` you might want to test for perf improvements (orthogonal to this patch, but could boost its numbers :). And also noted that `Lexer::getBuffer()` has same issue as `getBufferLocation()` re potential for pointer invalidation

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

tschuett added inline comments. Comment at: clang/include/clang/Lex/Lexer.h:89 - // End of the buffer. - const char *BufferEnd; + // Size of the buffer. + unsigned BufferSize; cor3ntin wrote: > Should that use `SourceLocation::UIntTy`? > Looking at comments

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

cor3ntin added a comment. This is an impressive amount of work. I think it makes sense! Thanks a lot for doing that work. I only have a few nits after a first review of this. Comment at: clang/include/clang/Lex/Lexer.h:89 - // End of the buffer. - const char *BufferEnd; +

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

davrec added a comment. Only had a chance to give it a once over, I will look through more closely later, def by this weekend. Main thing is I think we shouldn't be exposing the buffer pointers after this change, i.e. no public function should return `const char *`, unless I'm missing somethin

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

v.g.vassilev added a comment. We should probably add some tests here. Alternatively we can add the tests from https://reviews.llvm.org/D143148 but that'd make this patch bulkier and probably harder to review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

sunho added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:211 + L->BufferOffset = + StrData - InputFile.getBufferStart(); // FIXME: this is wrong + L->BufferSize = L->BufferOffset + TokLen; v.g.vassilev wrote: > Is that an outdated comment? If not m

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

v.g.vassilev added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:211 + L->BufferOffset = + StrData - InputFile.getBufferStart(); // FIXME: this is wrong + L->BufferSize = L->BufferOffset + TokLen; Is that an outdated comment? If not maybe elaborate

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

shafik added reviewers: aaron.ballman, cor3ntin, tahonermann. shafik added a comment. WG21 is meeting all this week, so a bunch of folks who should take a look at this may not get around to it right away. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

shafik added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:1203 if (L && !L->isLexingRawMode()) - L->Diag(CP-2, diag::trigraph_ignored); + L->Diag(CP - 2 - L->getBuffer().data(), diag::trigraph_ignored); return 0; I wonder do we really