Add Clone bound to BitIter in Jet::decode()#343
Add Clone bound to BitIter in Jet::decode()#343topologoanatom wants to merge 1 commit intoBlockstreamResearch:masterfrom
Clone bound to BitIter in Jet::decode()#343Conversation
| } | ||
|
|
||
| pub fn decode_expression<'brand, I: Iterator<Item = u8>, J: Jet>( | ||
| pub fn decode_expression<'brand, I: Iterator<Item = u8> + Clone, J: Jet>( |
There was a problem hiding this comment.
In 2db8d12:
This bound is not necessary. Neither are any of the other bounds you added.
There was a problem hiding this comment.
Could you elaborate on why the bound is not necessary? Can the related issue be solved without adding the bound, or are you referring to bounds other than the one in the Jet definition? If the latter, I added the minimal number of bounds required for the code to compile.
There was a problem hiding this comment.
The function worked without the bound -- adding the bound tightens it, but nothing in the function uses the extra bound. Doing this makes the API less useful, not more useful, because it reduces the number of ways you can call these functions.
The PR description claims that it adds a bound to BitIter but it does not.
There was a problem hiding this comment.
but nothing in the function uses the extra bound.
If you're referring to the function in the second code block of the issue description, that's pseudocode to illustrate the motivation. The actual implementation uses clones.
The PR description claims that it adds a bound to BitIter but it does not.
BitIter already derives Clone, so to obtain cloneable iterators, we need to bound the iterator type parameter. Moreover, adding the bound directly to BitIter itself would require a substantial number of changes (possibly all of them) in this codebase, plus changes to the autogenerated jet::init:: files. Sorry if the description was confusing, but this approach seems like the minimal intervention to achieve the issue's goal.
If the existing BitIter API provides a way to rewind steps that I somehow missed, please point it out. Otherwise, is the issue's motivation worthy of restricting bounds?
|
Please don't submit slop to this repo. |
|
Closing. The |
2 participants
Fixes #342