grep: support POSIX equivalence classes in brackets - #109
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
lhecker
left a comment
lhecker
left a comment
There was a problem hiding this comment.
Thank you for your work.
| /// A class whose body is not exactly one character is left in place, as is one | ||
| /// holding `]`, `^`, `-` or `\`, whose meaning inside a bracket expression | ||
| /// depends on where it sits. | ||
| fn rewrite_equivalence_classes(pattern: &str) -> Option<String> { |
There was a problem hiding this comment.
If you return a Cow the code above becomes a lot more pleasant to read.
| /// Rewrite POSIX equivalence classes to their bare member, e.g. `[[=a=]b]` to | ||
| /// `[ab]`. In the C locale an equivalence class holds only the character | ||
| /// itself, and oniguruma has no syntax for it at all. Returns `None` when the | ||
| /// pattern has nothing to rewrite. | ||
| /// | ||
| /// A class whose body is not exactly one character is left in place, as is one | ||
| /// holding `]`, `^`, `-` or `\`, whose meaning inside a bracket expression | ||
| /// depends on where it sits. |
There was a problem hiding this comment.
The comment is very informative (which is great), but I'm not entirely sure I understand the wording per se. The code below is rather complex so a good function comment goes a long way. It doesn't need to explain what the code does of course, but rather its reason for existence. An example of a regex translation (or multiple) for instance may help.
| b'\\' => i += 2, | ||
| b'[' => { |
There was a problem hiding this comment.
This code adopts the same unideal approach to scanning already used by has_confusing_bracket where a byte-wise loop is used as opposed to a memchr style next-significant-character search (in this case that's [; a look-behind can clear up whether \ precedes it). You can choose to clean up both functions but keep them identical for consistency.
Consistency in this case means the complex inner part should be hoisted into its own function. And it would be very nice if the inner part wasn't this nested and complex.
5d1bf3d to
dc0b429
Compare
Both has_confusing_bracket and rewrite_equivalence_classes walked the pattern one byte at a time looking for '['. Only '[' is significant between bracket expressions, so share a next_unescaped_bracket helper that jumps to the next one and decides escaping from the parity of the backslash run in front of it. Keeps the two functions structurally identical, as before.
|
Thanks for the review — pushed the scanning cleanup. Both Added two cases for the parity edge, since that is the easy thing to get wrong: On the other two points — |
| fn next_unescaped_bracket(pattern: &[u8], from: usize) -> Option<usize> { | ||
| let mut search = from; | ||
| while let Some(offset) = memchr(b'[', &pattern[search..]) { | ||
| let at = search + offset; | ||
| let mut backslashes = 0; | ||
| while at - backslashes > from && pattern[at - backslashes - 1] == b'\\' { | ||
| backslashes += 1; | ||
| } | ||
| if backslashes % 2 == 0 { | ||
| return Some(at); | ||
| } | ||
| search = at + 1; | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
For reference, what I had in mind was a scalar loop ("memchr style"):
fn next_unescaped_bracket_scalar(pattern: &[u8], mut from: usize) -> Option<usize> {
let mut escape = false;
while from < pattern.len() {
match pattern[from] {
b'[' => {
if !escape {
return Some(from);
}
}
b'\\' => escape = !escape,
_ => escape = false,
}
from += 1;
}
None
}Always keep in mind that LLMs were trained on tons of mostly average code, so the code they produce is similarly mostly average. Using a vectorized scanner on short inputs doesn't make much sense since the branch predictor just slaps it away. And the scalar loop produces less assembly.
| while let Some(open) = next_unescaped_bracket(pattern, i) { | ||
| let (confusing, next) = scan_bracket(pattern, open + 1); | ||
| if confusing { | ||
| return true; |
| /// inside a bracket expression depends on where it sits. | ||
| fn rewrite_equivalence_classes(pattern: &str) -> Cow<'_, str> { | ||
| let bytes = pattern.as_bytes(); | ||
| let mut out: Option<String> = None; |
There was a problem hiding this comment.
Init as String::new() (no Option), then return Borrowed(pattern) if out.is_empty().
| let bytes = pattern.as_bytes(); | ||
| let mut j = open + 1; | ||
| if bytes.get(j) == Some(&b'^') { | ||
| j += 1; | ||
| } | ||
| let body_start = j; | ||
| let mut out: Option<String> = None; | ||
| let mut copied = open + 1; |
There was a problem hiding this comment.
Can you ask an LLM to remodel this function based on first principles? I have an inkling it could be easier to read.
| } | ||
| return (out, j + 1); | ||
| } | ||
| // `[:`, `[.` and `[=` subexpressions inside the bracket. |
There was a problem hiding this comment.
Escape handling missing from what I can tell. Use next_unescaped_bracket. You'll need something like that for the closing bracket too. Perhaps make next_unescaped_bracket generic over the needle.
| && is_rewritable_equivalence( | ||
| &pattern[j + 2..end - 2], | ||
| bytes.get(end).copied(), | ||
| if j > 0 { Some(bytes[j - 1]) } else { None }, | ||
| ) |
There was a problem hiding this comment.
This is a hard read in particular.
2 participants
Fixes #35.
Oniguruma has no syntax for POSIX equivalence classes, so
[[=a=]]matched nothing and, worse, poisoned the surrounding bracket expression —[[=a=]b]stopped matchingbtoo.In the C locale an equivalence class holds only the character itself, so this rewrites
[=c=]to a barecinside bracket expressions before compiling. The walk reuses the existingfind_bracket_subexpr_endhelper and skips[:class:]/[.symbol.]so they are untouched.Three cases are deliberately left unrewritten, since a naive substitution would be wrong rather than merely unsupported:
[[=a=]-c]). GNU rejects these asinvalid character range; rewriting would silently produce a valid[a-c]. Left alone, so behavior is unchanged frommain.[[=ab=]],[[==]]). GNU reportsinvalid collating element.],^,-,\.Those still differ from GNU in that we do not report the error (exit 1, no diagnostic, exactly as on
main) — that is a pre-existing gap in bracket error reporting, not something this changes.Diffed 14 pattern shapes against GNU grep: the 11 matching-behavior cases now agree, and the 3 that remain are the error-reporting ones above, each returning main's existing exit 1.
Tests: unit tests for the rewrite itself, plus integration coverage for the class alone, combined with other bracket members, negated, under
-E, and taken literally under-F.cargo test: 95 integration + 15 unit passed. clippy-D warningsandcargo fmt --checkclean.