Implement address compilation#57
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces enhancements for address compilation in sed by adding a new module, additional functionality, and documentation updates.
- Added a new module (delimited_parser) in sed.rs
- Added a new "retreat" helper function to ScriptCharProvider
- Provided a default implementation for Command and updated the README with new extension details
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/uu/sed/src/sed.rs | New module added to support delimited parsing functionality. |
| src/uu/sed/src/script_char_provider.rs | Added a new retreat function to adjust the character position. |
| src/uu/sed/src/command.rs | Introduced a default implementation for Command. |
| README.md | Enhanced documentation with extension details. |
Comments suppressed due to low confidence (1)
src/uu/sed/src/script_char_provider.rs:32
- Consider adding unit tests for the new 'retreat' function to verify its behavior on boundary conditions (e.g., when n is greater than or equal to the current position).
pub fn retreat(&mut self, n: usize) {
dspinellis
force-pushed
the
compile-specs
branch
from
4cefe3d to
c650db5
Compare
This can also be used by parse_transliteration
To benefit from having recently worked on the regex code.
dspinellis
force-pushed
the
compile-specs
branch
from
c650db5 to
207fee2
Compare
|
I also added unit tests for the new 'retreat' function. |
This is mostly a placeholder for more unit tests as we add functionality to it.
| value: AddressValue::LineNumber(number), | ||
| }) | ||
| } | ||
| _ => panic!("invalid context address"), |
There was a problem hiding this comment.
panic a bit violent in rust :)
also, you might want to add the line info to help with debugging
There was a problem hiding this comment.
It's an internal error; it shouldn't happen. The code reaches this point only when is_address_char has detected one of the preceding characters.
| .and_then(char::from_u32) | ||
| { | ||
| Some(decoded) => Some(decoded), | ||
| None => panic!("Unable to decode numeric character escape."), |
There was a problem hiding this comment.
same comment about the panic :)
There was a problem hiding this comment.
Same here: preceding lines have already verified that this is a valid sequence for the specified radix. The panic should only be triggered if there's a code error not a user error.
| let mut c = x; | ||
| if c.is_ascii_lowercase() { | ||
| c = c.to_ascii_uppercase(); | ||
| } |
There was a problem hiding this comment.
maybe just do
| let mut c = x; | |
| if c.is_ascii_lowercase() { | |
| c = c.to_ascii_uppercase(); | |
| } | |
| let c = x.to_ascii_uppercase(); |
| if n > self.pos { | ||
| self.pos = 0; | ||
| } else { | ||
| self.pos -= n; | ||
| } |
There was a problem hiding this comment.
| if n > self.pos { | |
| self.pos = 0; | |
| } else { | |
| self.pos -= n; | |
| } | |
| self.pos = self.pos.saturating_sub(n); |
There was a problem hiding this comment.
Didn't know that; thanks! Fixed.
|
kudos for all the tests :) |
|
I pushed a fix addressing the comments that needed fixing. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================
==========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.