-
Notifications
You must be signed in to change notification settings - Fork 727
Conversation
Hi @wjwoodson, thanks for the fix - I see that the regression tests passed successfully, but I just checked your regex with pcre2test, and looks like it doesn't work:
Here is the old version:
I think we need more check. |
Hi @airween, I believe the |
I think you could drop the |
Actually, @airween is correct in that your pattern will not match e.g. fooA0xf00 (or %5cA0xf00dsdfdsa for the matter) because the \b after the [^\d]? needs a different type before the A (both o and A are in \w). That said, I think fooAOxf00 (or %5cA0xf00dsdfdsa) should not be matching. |
Yes |
I'm inclined to say so (drop the [^\d]?) but let's wait for other folks' input. |
ah, sorry, you're right, I totally forgot to check the rule. Sorry again. |
This looks like a good progress on this front. You guys understand much more of this than I do, but I'd lean on dropping the 942450 is a bit thin on tests. Here is some stuff to enhance the test suite. If you find it useful @wjwoodson, then it might pay to pick some, rework them a bit and add them to the unit tests. |
Hi all, I've updated to remove |
LGTM. |
Thanks for the PR, the update and the additional tests, @wjwoodson. Merging now. |
Update regexp in 942450 to reduce false positives. This is essentially the change to require
\b
before the potential hex string mentioned in #833.