Skip to content
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

tr: add full support for ranges #314

Conversation

andrewliebenow
Copy link
Contributor

tr currently only handles ranges if they are the only construct in
the string containing them (see contains_single_range).

Add proper handling of ranges: multiple in one string, ranges from
one character type to another (e.g. tr -d '\060-9 A-Z').

Fix handling of octal sequences above \377.

Add validation to ensure equiv (in "[=equiv=]" constructs) is only
one character.

Add validation to ensure ranges are not "backwards" (the ending
character be must greater than or equal to the starting character).

Parse and validate string1 and string2 before reading from stdin.

tr currently only handles ranges if they are the only construct in
the string containing them (see `contains_single_range`).

Add proper handling of ranges: multiple in one string, ranges from
one character type to another (e.g. tr -d '\060-9 A-Z').

Fix handling of octal sequences above \377.

Add validation to ensure equiv (in "[=equiv=]" constructs) is only
one character.

Add validation to ensure ranges are not "backwards" (the ending
character be must greater than or equal to the starting character).

Parse and validate string1 and string2 before reading from stdin.
@andrewliebenow andrewliebenow force-pushed the tr-add-full-support-for-ranges-rebased branch from 3b6964e to 0d821e0 Compare October 13, 2024 07:24
Copy link
Contributor

@jgarzik jgarzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jgarzik
Copy link
Contributor

jgarzik commented Oct 13, 2024

Needs minor merge conflict resolution but overall looks like forward progress

@andrewliebenow andrewliebenow force-pushed the tr-add-full-support-for-ranges-rebased branch from 0d821e0 to d2c6c48 Compare October 14, 2024 00:26
@andrewliebenow
Copy link
Contributor Author

  • better performance (all operations are now streaming, so they run faster and use a constant amount of memory)
  • fixes some more argument parsing bugs
  • adds almost 1,000 lines of code (it looks like around 250 of those are new tests, though)
  • possibly more confusing implementation: for performance, multiple operations (like delete and squeezed) are fused, but this results in less straightforward code

@jgarzik jgarzik merged commit 47bb77f into rustcoreutils:main Oct 14, 2024
2 checks passed
@andrewliebenow
Copy link
Contributor Author

I meant for those points to be rendered as +'s and -s':

+ better performance (all operations are now streaming, so they run faster and use a constant amount of memory)
+ fixes some more argument parsing bugs
- adds almost 1,000 lines of code (it looks like around 250 of those are new tests, though)
- possibly more confusing implementation: for performance, multiple operations (like delete and squeezed) are fused, but this results in less straightforward code

There are still many improvements that could be made here. I should have some more PRs for those soonish. I will keep an eye out for reports of bugs/regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants