-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix bug in GET_PREV_CHAR macro #278
Conversation
- pass `cbuf_type` variable to XXX_CHAR macros in `lre_exec_backtrack()` - improve readability of these macros - fix GET_PREV_CHAR macro: cptr was decrementes twice on invalid high surrogate.
I found this while proof reading as I was merging some of the differences into quickjs. It is difficult to create a test case triggering this bug... |
libregexp.c
Outdated
if (_p < _end && is_lo_surrogate(*_p)) { \ | ||
c = from_surrogate(c, *_p++); \ | ||
} \ | ||
} \ |
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.
Can you leave out stylistic changes? They make the diff larger and more difficult to read.
As a reviewer, I first have to figure out if it's actually a functional change - and if not, if it's really equivalent to the old code. As a maintainer, style changes introduce churn that makes git blame
less effective. As a contributor, it's rage inducing when your carefully crafted pull request ends up with a ton of merge conflicts because someone landed a style change the other day.
Changes that improve legibility are okay but a) sparingly, and b) should be split off into separate commits and marked NFC (Non-Functional Change.)
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.
Sorry about that, I was trying to reduce the macro size and combine the tests for consistency. I posted a patch with minimal NFC differences.
- pass `cbuf_type` variable to XXX_CHAR macros in `lre_exec_backtrack()` - improve readability of these macros - fix GET_PREV_CHAR macro: cptr was decrementes twice on invalid high surrogate. - minimize non functional changes
I am going to merge this patch. This is work in progress:
|
cbuf_type
variable to XXX_CHAR macros inlre_exec_backtrack()