-
Notifications
You must be signed in to change notification settings - Fork 10.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow common process_escapes to handle \x sequences #3928
Allow common process_escapes to handle \x sequences #3928
Conversation
common/common.cpp
Outdated
const char x[3] = { input[input_idx + 1], input[input_idx + 2], 0 }; | ||
char *err_p = nullptr; | ||
const long val = std::strtol(x, &err_p, 16); | ||
if (*err_p == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is C++, why not something like this?
const char x[3] = { input[input_idx + 1], input[input_idx + 2], 0 }; | |
char *err_p = nullptr; | |
const long val = std::strtol(x, &err_p, 16); | |
if (*err_p == 0) { | |
std::string x(input + input_idx + 1, 2); | |
size_t pos = 0; | |
const long val = std::stol(x, &pos, 16); | |
if (pos == x.length()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the first reason is I didn't know to write it that way. :) I think there's a reason to keep it as-is though, creating a const 3 character array probably has lower overhead than making and destroying a std::string
, concatenating some other strings, etc. It's probably a small difference, but hey, when I'm feeding a 100,000 token prompt to a 128K YaRN model maybe it'll be .5sec faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of small-string optimization there won't be any heap allocation, but I suppose std::string is still slightly more expensive.
If we keep the C version, we should replace const char x[3]
with just const char x[]
since the size is inferred.
C++ has had std::string_view and std::from_chars since 2017, but we can't use them :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized the previous code didn't correctly handle a case where the second hex digit was \0
. This version is a little simpler and clearer also. However, I kept the [3]
since I feel better about explicitly setting the size when doing pointer arithmetic on the array like that.
* Allow common process_escapes to handle \x sequences * Fix edge case when second hex digit is NUL
For example, you can do something like
\xf0\x9f\x98\x82
to enter 😂 with these changes.