Implement missing TrustedLen for core and alloc types - #160838
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Implement missing TrustedLen and FusedIterator for core and alloc types
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9a3fc0e): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary -1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 454.914s -> 461.019s (1.34%) |
|
I think the TrustedLen impls are not observable on stable (outside of better performance in some cases), so while we'll need some review to ensure correctness, those seem fine. However, implementing FusedIterator is a stable guarantee, so we'll definitely need to get libs FCP on that - I'd split that out into a separate PR. I think the format that would make the most sense for approving it is something like this: with any relevant conditions on the generics included. That should focus on the public API rather than any internal changes needed to facilitate that. That can then be FCPd. Note that for those structures that delegate their iterator implementation, I think we should make sure that we have a static assertion that the inner iterator type is also a FusedIterator. Any platforms where we can't consistently provide FusedIterator (I'd hope none, but let's check) should be separated out into a separate PR.
Is there a Clippy lint for this? I suspect the vast majority of Iterator types want to be Fused, and the vast majority of ExactSize iterators can be TrustedLen. It seems like we ought to catch that earlier if possible, and a Clippy and/or rustc lint for it would make sense to me. (Obviously in the cases that it doesn't make sense we can allow the lint). |
2b8206a to
b6d8f9a
Compare
Thank you for your guidance, I didn't know that, I removed FusedIterator implementations, once we merged this PR, I'll start sending separate PRs for each missing
I checked and didn't find any rule related to this. I wrote a simple naive python script that reads formatted rust. |
|
@rustbot ready |
|
Can you update title + description + squash commits? I'd also like a second look from @the8472 in case I missed anything for the impls here, given they are making new soundness (but not correctness) guarantees, including whether we need FCP -- I think no, because TrustedLen is unstable, but not sure if it's actually exposed somewhere. |
|
Thank you, I did most of it but I'll wait for the review then I will squash commits to avoid multiple CI runs. |
While working on #160813 I noticed TrustedLen is missing for the LinkedList, I suspected that other modules are missing the implementation too, so I wrote a small script to scan the project and found these candidates which implement ExactSizeIterator but not the TrustedLen, Reviewed the code just to make sure they are actually meet TrustedLen requirement and then I've added missing implementations.
While editting I found out there are instances of FusedIterator missing too,
So I've added them too.I think most important missing one is the
impl<I: TrustedLen + ?Sized, A: Allocator> TrustedLen for Box<I, A>which inheritsTrustedLenfor aBoxto aTrustedLenimplementer.I did this only for
coreandalloc, I ignored script reports for thestdbut I think it worth looking into, specially for theHashMap:This is related to #37572