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 something. If that box is checked and performance
cost is negligible I'd give this the thumbs up.
================
Comment at: clang/include/clang/Lex/Lexer.h:307
/// Return the current location in the buffer.
- const char *getBufferLocation() const { return BufferPtr; }
+ const char *getBufferLocation() const {
+ assert(BufferOffset <= BufferSize && "Invalid buffer state");
----------------
I think I'd like this to return `unsigned`; i.e. I think after this patch we
should not even be publicly exposing buffer locations as pointers, IIUC. A
brief search for uses of `getBufferLocation()` (there aren't many) suggests
this would be probably be fine and indeed would get rid of some unnecessary
pointer arithmetic. And indeed if anything really needs that `const char *`
that might be a red flag to investigate further.
================
Comment at: clang/include/clang/Lex/Lexer.h:609
- bool CheckUnicodeWhitespace(Token &Result, uint32_t C, const char *CurPtr);
+ bool CheckUnicodeWhitespace(Token &Result, uint32_t C, unsigned CurOffset);
----------------
FWIW it sucks that `uint32_t` is already sprinkled throughout the interface
alongside `unsigned`, wish just one was used consistently, but that does not
need to be addressed in this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143142/new/
https://reviews.llvm.org/D143142
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
