-
Notifications
You must be signed in to change notification settings - Fork 848
Update header_rewrite to use Regex #12573
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
Conversation
|
[approve ci autest 1] |
bneradt
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.
Looks good.
We have to remember to remove PCRE::PCRE from our CMakeLists.txt files. I almost missed that in my url_sig PR.
|
[approve ci autest] |
I figured we would take pass after these are done to remove the PCRE dep and cleanup cmake. I can go ahead and remove it though. |
5a96ebe to
61fc7ce
Compare
| if (!re.setRegexMatch(s, has_modifier(mods, CondModifiers::MOD_NOCASE))) { | ||
| TSError("[%s] Invalid regex: failed to precompile: %s", PLUGIN_NAME, s.c_str()); | ||
| Dbg(pi_dbg_ctl, "Invalid regex: failed to precompile: %s", s.c_str()); | ||
| throw std::runtime_error("Malformed 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.
I assume we're removing the error since this is matching not pre-compiling. Is the Dbg log message wrong too then?
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 moved the error into the helper class so it could also print the regex error. Currently you only get the regex that failed, now you'll get why and what character too.
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.
Gotcha. Should we move the Dbg log there too though with the extra information.
|
[approve ci autest 3] |
bneradt
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
* Update header_rewrite to use Regex * PR review * cast to int * also move dbg for failed regex compile. (cherry picked from commit a15669b)
No description provided.