Repository navigation
Conversation
`s/x/y/2g` (replace the 2nd match and every later one) is valid in GNU sed, but was rejected with "multiple 'g' or numeric flags". Track `g` separately from the occurrence number, and report repeated flags with GNU's two messages: "multiple `g' options to `s' command" and "multiple number options to `s' command". The latter now points at the second number, e.g. the `3` in `2g3`, as GNU does. An invalid back-reference such as `s/\(a\)/\2/` was reported after the flags were read, so the diagnostic caret landed past the end of the command. Record where the highest `\N` appears and report it there.
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation and regression coverage support the intended behavior, with no unresolved findings.
No open issues 🎉
What changed in this PR
Improves sed’s GNU compatibility for substitution flags and makes error locations more precise.
Changes:
- Accepts
NgandgNto replace the Nth match and all later matches. - Uses GNU-style duplicate-flag messages and points diagnostics at offending flags or back-references.
- Adds regression tests for substitution behavior and diagnostic locations.
| File | Description |
|---|---|
| tests/by-util/test_sed.rs | Tests combined flags and diagnostic carets. |
| src/sed/processor.rs | Applies global replacement from the selected occurrence. |
| src/sed/compiler.rs | Parses combined flags and records precise error locations. |
| src/sed/command.rs | Stores global mode and back-reference locations. |
Review effort: Balanced
Merging this PR will improve performance by 7.68%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | number_fix |
1.3 s | 1.2 s | +12.08% |
| ⚡ | access_log_subst |
2.2 s | 2.1 s | +3.46% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sylvestre:subst-flag-diagnostics (294b1a1) with main (b1ea0dc)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #608 +/- ##
==========================================
- Coverage 94.85% 94.84% -0.01%
==========================================
Files 14 14
Lines 7230 7241 +11
Branches 432 432
==========================================
+ Hits 6858 6868 +10
- Misses 371 372 +1
Partials 1 1
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:
|
s/x/y/Ngands/x/y/gNare now accepted, like GNU: replace the Nth match and every one after. They were rejected with "multiple 'g' or numeric flags in substitute command" (and unit tests asserted that).multipleg' options tos' commandandmultiple number options tos' command. The latter points at the second number (3ins/a/b/2g3, char 9 like GNU) instead of at theg`.s/\(a\)/\2/) was reported after the flags were parsed, so theUUTILS_DIAGcaret sat past the end of the command. It now points at the\2, including when the replacement spans several lines.