-
Notifications
You must be signed in to change notification settings - Fork 242
Improve coverage of escapes in character classes #591
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
Improve coverage of escapes in character classes #591
Conversation
| #define RMATCH(ra,rb) \ | ||
| do { \ | ||
| start_ecode = ra; \ | ||
| Freturn_id = rb; \ | ||
| goto MATCH_RECURSE; \ | ||
| L_##rb:; \ | ||
| } \ | ||
| while (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.
All the calling code for the macros like RMATCH(...); uses a trailing semicolon.
The existing code was creating a block, not an expression, so the trailing semicolon was treated as a (harmless) empty statement. However, it was polluting the coverage statistics with unreached code lines, because those statements were never reached.
Using a do {...} while (0) is a good way to avoid surprises with macros.
79cac06 to
6297dd3
Compare
| /* All others are not allowed in a class */ | ||
|
|
||
| default: | ||
| PCRE2_DEBUG_UNREACHABLE(); |
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.
Not allowed or not possible?
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.
Not possible (as far as I can tell). I have gone through check_escape() and made a list of every ESC_ code it can return (when passed inclass=TRUE), and added a case statement and a test for all of those. There should be no other escapes allowed, so I added a catch-all default which only triggers in Debug builds.
zherczeg
left a comment
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.
LGTM
One switch case in pcre2_compile.c is not covered by any tests (the ERR7 default).
Add tests covering all the possible escapes, and fix a couple that weren't working (ESC_k and ESC_ub).
Also fix some empty statements causing coverage failures in pcre2_match.c.