-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add builder method for nest_limit
.
#454
Conversation
Mirrors the addition to regex-syntax, or there would be no way to set it.
) | ||
.nest_limit(1024) | ||
.build() | ||
.unwrap(); |
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.
Hmmm just from glancing it kind of looks like this regex isn't that deeply nested. I wonder if these is a bug in the logic that computes the nest level. What nest level is reported for this regex?
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.
How do I get the nest level? The error only mentions the maximum nest level (or I can't see it).
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.
Ah I see, then it's not exposed. Requires debugging. I'd like to explore that before merging this. With that said, even if your bug wasn't a thing, exposing the nest level seems right to me.
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, there are some really huge regular expressions in there, so I wouldn't be suprised if some of them went beyond the nest limit in case it is indeed a bug.
@meh It turns out this was indeed a (quite embarrassing!) bug and your regex definitely should not have tripped the limit. It's fixed on master. I should be able to get a new release out before the weekend is through. If not, please ping me. :-) |
@meh Errmm, I broke up your commit into two pieces but forgot to fix the author information. Sorry about that! :-( |
@BurntSushi no worries, as long as it's fixed 🐼 |
@BurntSushi release ping 🔔 |
@meh Thanks for the ping! I just put |
I didn't know where to put the test case but I think crazy is fairly accurate (it's one of thousands of regular expressions coming from libphonenumber).
Also I know this may sound silly, but could this get a version bump too please? The
phonenumber
crate cannot parse phone numbers anymore and it was through a minor bump, so I'm kind of stuck.