Add validations for brace quantifiers {m}, {m,} and {m,n}#328
Add validations for brace quantifiers {m}, {m,} and {m,n}#328LoukasPap wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
+ Coverage 82.18% 83.12% +0.93%
==========================================
Files 13 13
Lines 5551 5895 +344
Branches 312 340 +28
==========================================
+ Hits 4562 4900 +338
- Misses 986 992 +6
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Forgot to add unit tests. I will add soon. |
|
@e-kwsm Ready for review! 🙂 |
There was a problem hiding this comment.
{,n} to be parsed as literal
nitpick: "{,n}" pattern is not specified in ERE, but
<<< xxx sed -E -n -e '/x{,-1}/s//@/p'- GNU and BSD:
Invalid content of \{\}, exit with one - uutils: prints nothing and exits with zero
<<< xxx sed -E -n -e '/x{,0}/s//@/p'- GNU and BSD: print
@xxx - uutils: prints nothing
<<< xxx sed -E -n -e '/x{,1}/s//@/p'- GNU and BSD: print
@xx - uutils: prints nothing
<<< xxx sed -n -E -e '/.{,x}/p'
|
GNU and BSD support 255-repetition.
https://man.freebsd.org/cgi/man.cgi?re_format(7)
|
|
Nice catches, I'll check them |
LoukasPap
force-pushed
the
issue289
branch
2 times, most recently
from
85ba8bd to
47d16fe
Compare
|
I am making some changes and I will re-push. |
LoukasPap
force-pushed
the
issue289
branch
2 times, most recently
from
b5fa74b to
d650a80
Compare
LoukasPap
changed the title
|
There were many approaches one could follow to add validation for the quantifiers, each with its own advantages and disadvantages. What I did:
Other solutions could be to break the parsing to more places in code by adding more match cases (comma and digit), or to contain the entire validation in one function (that's what I had done initially but I though it was too complicated). Another solution could possibly be to even use regex for validating the regex quantifier. |
|
@sylvestre Ready for review 🙂 (I'll squash before merge) |
|
I will make changes in the next few days and re-push. |
LoukasPap
force-pushed
the
issue289
branch
2 times, most recently
from
0eb1673 to
461c1d5
Compare
|
Just rebased atop main and squashed all commits into one. I am open to new approaches and suggestions for a better solving of the problem, if it's needed :) |
| } | ||
| let m_val: u32 = match m.parse() { | ||
| Ok(val) => { | ||
| if val > 255 { |
There was a problem hiding this comment.
Upper bound is wrong — 255 should be 32767 (RE_DUP_MAX). no ?
There was a problem hiding this comment.
I had seen here that the value is 255, but 32767 is the upper limit as I tested it for both GNU and POSIX, so you are right.
| && is_address_char(line.current()) | ||
| && let Ok(addr1) = compile_address(lines, line, context) | ||
| { | ||
| if !line.eol() && is_address_char(line.current()) { |
There was a problem hiding this comment.
If we keep it as it was, errors are being silently skipped without back-propagating them. Moving compile_address(lines, line, context)?; in a separate line catches compilation errors. Or did you mean something else?
3 participants
Fixes issue 289
There are two types of errors in quantifiers:
Unmatched \{for unterminated bracesRegular expression too bigif m and/or n are outside the [0,255] boundsInvalid content of \{\}for all the othersUnmatched \{sedalways captures the unmatched braces error first. To deal with that, I scan ahead the regex for unclosed{ }and if it is violated it throws the error, otherwise we retreat back to the beginning to start the processing.Regular expression too bigInvalid content of \{\}It checks for:
{,}to be converted to0,{,n}to be converted to{0,n}