Followup to #895: rename dissat_sat function and use struct rather than tuple for return values#930
Followup to #895: rename dissat_sat function and use struct rather than tuple for return values#930apoelstra wants to merge 2 commits intorust-bitcoin:masterfrom
dissat_sat function and use struct rather than tuple for return values#930Conversation
|
On 5097641 successfully ran local tests |
| let (dissats, sats): (Vec<_>, Vec<_>) = sat_dissats | ||
| .into_iter() | ||
| .map(|SatDissat { dissat, sat }| (dissat, sat)) | ||
| .unzip(); |
There was a problem hiding this comment.
let (dissats, sats): (Vec<_>, Vec<_>) = stack
.drain(stack.len() - thresh.n()..)
.map(|SatDissat { dissat, sat }| (dissat, sat))
.unzip();
There was a problem hiding this comment.
Ah, yeah, I should combine these two let lines.
There was a problem hiding this comment.
I think in an earlier iteration of this PR I was still using the sat_dissat variable, but clearly I am not now..
There was a problem hiding this comment.
Heh, and I don't think my "allocation-wasteful" comment actually applies anymore. Nice.
| } | ||
| } | ||
|
|
||
| /// The (dissatisfaction, satisfaction) pair for an `older` fragment. |
There was a problem hiding this comment.
minor nit, can ignore: Find and replace on "(dissatisfaction, satisfaction) pair" to just "satisfaction and dissatisfaction" or "SatDissat" in the doc comments.
apoelstra
force-pushed
the
2026-04/followup-895
branch
from
5097641 to
5870086
Compare
|
Fixed both nits. |
| // order of our tuple. | ||
| let (dissats, sats): (Vec<_>, Vec<_>) = stack | ||
| .drain(stack.len() - thresh.n()..) | ||
| .into_iter() |
There was a problem hiding this comment.
Any reason for still using into_iter()? It compiled without it for me.
There was a problem hiding this comment.
Ah clippy picked up on it too.
There was a problem hiding this comment.
Hah, nice, we're saving LOC all over the place. Fixed.
It is too easy to get the satisfactions and dissatisfactions confused. Use a struct with named fields. Curiously, outside of this module dissatisfactions are never used, so we can keep the `dissat` field private. (Well, this is a bit of a sketchy thing to say -- dissatisfactions **are** used by the Satisfaction::thresh and thresh_mall functions, but those functions take dissats and sats as independent Vecs they can manipulate, rather than taking a SatDissat struct.)
The name of the function no longer implies its return value (which is a struct rather than a tuple that needs to be carefully ordered). So change the name to be consistent with the name of the module.
apoelstra
force-pushed
the
2026-04/followup-895
branch
from
5870086 to
f7f1689
Compare
|
On f7f1689 successfully ran local tests |
2 participants
Makes naming more consistent and reduces confusion caused by having a tuple with two entries of the same type.