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