-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
extmod/re1.5: Fix escaping in RE classes. #5221
Conversation
if (*re == '\\') { | ||
++re; | ||
if (!*re) return NULL; | ||
} |
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.
ok, that's much simpler to fix than I was expecting!
Looks good to me |
extmod/re1.5/compilecode.c
Outdated
@@ -54,6 +54,10 @@ static const char *_compilecode(const char *re, ByteProg *prog, int sizecode) | |||
prog->len++; | |||
for (cnt = 0; *re != ']'; re++, cnt++) { | |||
if (!*re) return NULL; | |||
if (*re == '\\') { | |||
++re; | |||
if (!*re) return NULL; |
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 know the rest of the file uses this style as well, but new code should probably adhere to the hous style i.e. use brackets?
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.
Considering this file is from an external module, I would think it best to maintain the existing style.
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.
Yeah, not sure, sort of depends on my other question about the relation to upstream, if any.
Anyway: the code can be simplified I think, move the increment before the existing NULL check and only one check is needed:
for (cnt = 0; *re != ']'; re++, cnt++) {
if (*re == '\\') {
++re;
}
if (!*re) return NULL;
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.
Yes, good catch! Thanks
Regarding upstream... it already has diverged.
My intention was to match the existing code style.
Not sure what the status is of the code here vs where it originally came from (pfalcon's repository I guess?) but ideally this gets fixed there as well? |
Fixes micropython#3178 and micropython#5220 Adds tests, including all the cases mentioned in both bugs.
Support for escape within Merged in ebf8332 |
…io-fontio-mp_register_module Use MP_REGISTER_MODULE with displayio, terminalio, and fontio
Fixes #3178 and #5220
Adds tests, including all the cases mentioned in both bugs.