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

[Tom Honermann via Phabricator via cfe-commits]

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 <https://reviews.llvm.org/D143142#4142212>, @aaron.ballman wrote:

> In terms of whether to use `unsigned` vs a specific type; I don't have a strong opinion, but it'd be good to at least `static_assert` properties we care about (like the type being at least 32 bits). We've had efforts in the past to allow for > 2GB source files, so I weakly think it would make sense to use a 64-bit value explicitly up front, but I have no idea how that changes the performance characteristics for the changes either.

I like the idea of using a strong type for two reasons: 1) normal type safety stuff, and 2) it would enable a platform dependent and/or configurable type; I don't see any reason for use of a 64-bit offset on an ILP-32 platform, so no point in adding the overhead there.

> In terms of whether to expose access to pointers to the underlying buffer through lexer APIs... I sort of agree with @davrec that it would be good to avoid exposing those interfaces given that the pointer values are likely to be invalidated when the buffer grows. My instinct is that Clang developers are unlikely to consider that buffer to be something that can be invalidated, so if we retain an interface to get a pointer to the buffer when you get to the point of actually allowing it to grow, it'd  be nice if we can find some way to forcefully grow the buffer to help catch misuses in Clang when running the llvm-lit tests.

Perhaps such access can be facilitated via a `shared_ptr`-like handle that dynamically records whether direct access is in use; something like a read-lock. Attempts to grow the buffer could then assert that no such use is outstanding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143142