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 support for octal sequences #306

Merged

Conversation

andrewliebenow
Copy link
Contributor

Also: fix string1 order bug

Also: fix string1 order bug
@andrewliebenow
Copy link
Contributor Author

Note: these changes allow the gmp-mpfr-sys crate to be built using posixutil's tr (its configuration script calls tr)

@jgarzik
Copy link
Contributor

jgarzik commented Oct 10, 2024

Looks OK to me, but I haven't tested yet.

Questions:

  1. Is the Debug derivation required? I prefer to remove where not absolutely required, to minimize compile time.
  2. Can you do some quick micro-benchmarking, with and without this PR? Just a quick comparable.

@andrewliebenow andrewliebenow force-pushed the tr-add-support-for-octal-sequences branch from 3dd7c3e to 6360e4e Compare October 10, 2024 20:29
@andrewliebenow andrewliebenow force-pushed the tr-add-support-for-octal-sequences branch from 6360e4e to 9237013 Compare October 10, 2024 20:32
@andrewliebenow
Copy link
Contributor Author

The Debug derives were an accident. I added them while I was... debugging. I can do some benchmarking. I think performance should be better, at least in the plain, no options case, because generate_transformation_hash_map does all the work figuring out how characters should be transformed (or not), and that's a one time cost. Transforming the input is just a loop over the input chars with an index into a HashMap. Before, the logic of determining how an input should be modified was mixed in with the actual transformation.

@jgarzik jgarzik merged commit 536319b into rustcoreutils:main Oct 10, 2024
2 checks passed
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