Implement size_hint for several unicode-based iterators. #50208

Phlosioneer · 2018-04-25T03:54:04Z

This continues work on issue #49205.

This PR also implements ExactSizeIterator and FusedIterator where
they make sense.

I didn't know what to put for the issue number when making the [unstable] annotation, so I put 0 as a placeholder.

This continues work on issue rust-lang#49205. This PR also implements ExactSizeIterator and FusedIterator where they make sense.

rust-highfive · 2018-04-25T03:54:07Z

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2018-04-25T03:59:06Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:02:58]    Compiling cc v1.0.10
[00:02:58]    Compiling core v0.0.0 (file:///checkout/src/libcore)
[00:02:58]    Compiling unwind v0.0.0 (file:///checkout/src/libunwind)
[00:02:58]    Compiling build_helper v0.1.0 (file:///checkout/src/build_helper)
[00:03:01] error[E0405]: cannot find trait `FusedIterator` in this scope
[00:03:01]    --> libcore/str/lossy.rs:184:6
[00:03:01]     |
[00:03:01] 184 | impl FusedIterator for Utf8LossyChunksIter {}
[00:03:01]     |      ^^^^^^^^^^^^^ not found in this scope
[00:03:01] help: possible candidate is found in another module, you can import it into scope
[00:03:01] 11  | use iter::traits::FusedIterator;
[00:03:01]     |
[00:03:01] 
[00:03:01]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:03:01]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:03:04] error[E0106]: missing lifetime specifier
[00:03:04]    --> libcore/str/lossy.rs:184:24
[00:03:04]     |
[00:03:04] 184 | impl FusedIterator for Utf8LossyChunksIter {}
[00:03:04]     |                        ^^^^^^^^^^^^^^^^^^^ expected lifetime parameter
[00:03:04] error: aborting due to 2 previous errors
[00:03:04] 
[00:03:04] Some errors occurred: E0106, E0405.
[00:03:04] For more information about an error, try `rustc --explain E0106`.
---
[00:03:05] travis_fold:end:stage0-std

[00:03:05] travis_time:end:stage0-std:start=1524628725710163325,finish=1524628733057800408,duration=7347637083

[00:03:05] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:05] expected success, got: exit code: 101
[00:03:05] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:03:05] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:05] Build completed unsuccessfully in 0:00:08
[00:03:05] Build completed unsuccessfully in 0:00:08
[00:03:05] make: *** [tidy] Error 1
[00:03:05] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:116c0c88
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

scottmcm · 2018-04-26T01:06:35Z

impls are insta-stable, so you probably just mark them stable as of the next release.

shepmaster · 2018-04-26T02:17:37Z

r? @scottmcm

scottmcm · 2018-04-26T02:21:27Z

@shepmaster As far as I know, I don't actually have bors permissions, so I don't think I should be assigned?

shepmaster · 2018-04-26T19:30:34Z

@scottmcm I can delegate permissions to you if you'd like, but I can also pick someone else 😉 Which would you prefer?

shepmaster · 2018-04-26T19:36:18Z

src/libcore/str/lossy.rs

+    }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        (0, Some(self.source.len()))


The idea here is that we could have 0 due to an error, and self.source.len() if every character is ASCII?

Side note: I wonder if we should have small comments on all these describing the logic and rationale behind them, just like we encourage for unsafe blocks.

Yes, that's the idea. I'll add a comment about it.

scottmcm

This looks overall fine, though of course the tidy errors need to be fixed.

But it's adding insta-stable impls, so I think it needs @rust-lang/libs eyes on it.

scottmcm · 2018-04-30T07:00:10Z

src/libcore/char/mod.rs

 }

 #[stable(feature = "fused", since = "1.26.0")]
 impl FusedIterator for ToUppercase {}

+impl ExactSizeIterator for ToUppercase {}


Like the line above it, this should have an attribute, something like

#[stable(feature = "case_mapping_len", since = "1.27.0")]

scottmcm · 2018-04-30T07:02:38Z

src/libcore/char/mod.rs

+            CaseMappingIter::One(_) => {
+                (1, Some(1))
+            }
+            CaseMappingIter::Zero => (0, Some(0))


While I'm confident this is correct, I think it'd still be good to have a test to pin the behaviour; a bunch of things like assert_eq!('a'.to_lowercase().len(), 1), perhaps.

scottmcm · 2018-04-30T07:07:15Z

src/libcore/str/lossy.rs

@@ -177,6 +184,8 @@ impl fmt::Display for Utf8Lossy {
    }
 }

+impl<'a> FusedIterator for Utf8LossyChunksIter<'a> {}


Are there tests that verify this? (There might be something already; I dunno.)

alexcrichton · 2018-04-30T22:40:18Z

If new insta-stable impls are added, can tests also be added to ensure that they're correct? Otherwise would it be possible to leave them to a future PR?

shepmaster · 2018-05-06T22:14:43Z

Ping from triage, @Phlosioneer ! You've got some more feedback to address.

pietroalbini · 2018-05-14T09:46:05Z

Thank you for this PR @Phlosioneer! Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

Implement size_hint for several unicode-based iterators.

8d21e50

This continues work on issue rust-lang#49205. This PR also implements ExactSizeIterator and FusedIterator where they make sense.

rust-highfive assigned shepmaster Apr 25, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2018

Phlosioneer mentioned this pull request Apr 25, 2018

Tracking issue for missing Iterator::size_hint implementations #49205

Closed

62 tasks

Fix missing use statement

97e7466

rust-highfive assigned scottmcm and unassigned shepmaster Apr 26, 2018

shepmaster reviewed Apr 26, 2018

View reviewed changes

Add a comment, remove unstable annotation

12c0361

scottmcm reviewed Apr 30, 2018

View reviewed changes

scottmcm assigned SimonSapin and unassigned scottmcm Apr 30, 2018

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2018

pietroalbini closed this May 14, 2018

pietroalbini added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement size_hint for several unicode-based iterators. #50208

Implement size_hint for several unicode-based iterators. #50208

Phlosioneer commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

scottmcm commented Apr 26, 2018

shepmaster commented Apr 26, 2018

scottmcm commented Apr 26, 2018

shepmaster commented Apr 26, 2018

shepmaster Apr 26, 2018

Phlosioneer Apr 26, 2018

scottmcm left a comment

scottmcm Apr 30, 2018

scottmcm Apr 30, 2018

scottmcm Apr 30, 2018

alexcrichton commented Apr 30, 2018

shepmaster commented May 6, 2018

pietroalbini commented May 14, 2018

Implement size_hint for several unicode-based iterators. #50208

Implement size_hint for several unicode-based iterators. #50208

Conversation

Phlosioneer commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

scottmcm commented Apr 26, 2018

shepmaster commented Apr 26, 2018

scottmcm commented Apr 26, 2018

shepmaster commented Apr 26, 2018

shepmaster Apr 26, 2018

Choose a reason for hiding this comment

Phlosioneer Apr 26, 2018

Choose a reason for hiding this comment

scottmcm left a comment

Choose a reason for hiding this comment

scottmcm Apr 30, 2018

Choose a reason for hiding this comment

scottmcm Apr 30, 2018

Choose a reason for hiding this comment

scottmcm Apr 30, 2018

Choose a reason for hiding this comment

alexcrichton commented Apr 30, 2018

shepmaster commented May 6, 2018

pietroalbini commented May 14, 2018