split: use iterator to produce filenames #2868
Conversation
src/uu/split/src/filenames.rs
Outdated
| self.number.increment().ok()?; | ||
| } | ||
| Some(format!("{}{}{}", prefix, self.number, suffix)) |
There was a problem hiding this comment.
The FilenameIterator repeatedly increments a counter stored in self.number and formats it into a string with a specified prefix and suffix.
| /// number would require more digits than are available with the | ||
| /// specified width, then this method returns [`Err(Overflow)`]. | ||
| fn increment(&mut self) -> Result<(), Overflow> { | ||
| for i in (0..self.digits.len()).rev() { |
There was a problem hiding this comment.
To increment the number, add 1 to each digit from least significant digit to most significant digit, carrying the 1 if necessary. An overflow results in an error for FixedWidthNumber.
| } | ||
| } | ||
|
|
||
| // If the most significant digit is at its maximum value, then |
There was a problem hiding this comment.
For DynamicWidthNumber, an overflow results in resetting the digits to zero and increasing the number of digits by one.
| let digits: String = self.digits.iter().map(|d| (b'a' + d) as char).collect(); | ||
| write!( | ||
| f, | ||
| "{empty:z<num_fill_chars$}{digits}", |
There was a problem hiding this comment.
For example, the number whose digits are vec![1, 2, 3] gets displayed as "zbcd".
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
You went above and beyond with this. If only the entire codebase was this well made, tested and documented. It's fantastic! Thank you!
|
indeed but any idea why we are regressing with the gnu testsuite ? :) |
|
That is indeed strange, it seems to be the And this branch is not behind master, so I'm not sure what's going wrong. It might have something to do with the fact that the CI upgraded to Rust 1.58. Edit: I really have no clue why this happened. The output for 1.58 and 1.57 for |
|
Don't worry about the clippy lints, I fixed them in #2869 |
|
The same issue with |
jfinkels
force-pushed
the
split-filename-iterator
branch
2 times, most recently
from
faf85df to
98d142b
Compare
jfinkels
force-pushed
the
split-filename-iterator
branch
2 times, most recently
from
7b560bc to
d1875e6
Compare
|
Hi! Could you fix the conflicts? |
Replace the `FilenameFactory` with `FilenameIterator` and calls to `FilenameFactory::make()` with calls to `FilenameIterator::next()`. We did not need the fully generality of being able to produce the filename for an arbitrary chunk index. Instead we need only iterate over filenames one after another. This allows for a less mathematically dense algorithm that is easier to understand and maintain. Furthermore, it can be connected to some familiar concepts from the representation of numbers as a sequence of digits. This does not change the behavior of the `split` program, just the implementation of how filenames are produced. Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>
jfinkels
force-pushed
the
split-filename-iterator
branch
from
d1875e6 to
a5b435d
Compare
|
Thanks! |
3 participants
This pull request replaces the
FilenameFactoryintroduced in pull request #2859 withFilenameIteratorand calls toFilenameFactory::make()with calls toFilenameIterator::next(). We did not need the fully generality of being able to produce the filename for an arbitrary chunk index. Instead we need only iterate over filenames one after another. This allows for a less mathematically dense algorithm that is easier to understand and maintain. Furthermore, it can be connected to some familiar concepts from the representation of numbers as a sequence of digits.The most important part of this code is in the
FixedWidthNumber::increment()andDynamicWidthNumber::increment()methods, and in theDisplayimplementation for each of those structs.This does not change the behavior of the
splitprogram, just the implementation of how filenames are produced.I've adapted the algorithm suggested by @tertsdiepraam here: #2859 (comment) and set the
Co-authored-by:line in the commit message accordingly. Thanks a lot for the suggestion!