DoubleEndedIterator for Args #33312
DoubleEndedIterator for Args #33312
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Looks good to me, thanks for the PR! As you mentioned, can you share code in the Windows implementation to deduplicate, and then can you also add a few tests for this? |
|
Thanks @alexcrichton for the hints ! Currently I am trying to see how to best run the tests. For now, the fasted run I got was via Do you know how to speed it up testing in my case ? |
|
Never mind - I ran |
|
Another option may be the |
| } | ||
| } | ||
|
|
||
| #[stable(feature = "env", since = "1.0.0")] |
There was a problem hiding this comment.
This should be marked since = "1.11.0", not 1.0.0
|
Can you add on |
|
@sfackler Good idea, ExactSizeIterator shall be done as well ! |
|
@sfackler When running I assume that I have to create a new feature and assign it a since 1.11 accordingly. If so, what is the feature name you would like to see here ? |
|
I think this PR is ready for review. Please note that I am not sure my feature-name is desirable. Also I found ExactSizeIterator to be implemented already. |
| assert!(args[l-1].contains("stdtest")); | ||
| assert_eq!(args[1], "--logfile"); | ||
| assert!(args[0].ends_with(".log")); | ||
| } |
There was a problem hiding this comment.
Could you add a unit test elsewhere with this test as this'd unfortunately break running the binary manually :(
There was a problem hiding this comment.
(e.g., adding a test to src/test/run-pass where the test just executes itself to run the test)
|
Ah yeah the test name is fine, we tend to just pick totally arbitrary names anyway :) |
|
Thanks for the feedback ! The problem I see with the run-pass tests is the inability to pass arguments to the program, making it impossible to be sure the feature is implemented correctly. Do you have an idea how that could be done ? |
|
Ah yeah the trick I normally take looks like this test where a test re-executes itself to run the test and then asserts a property of the output |
|
Ok, it seems to be done :) ! A few things I noticed:
|
| @@ -271,6 +271,7 @@ | |||
| #![feature(vec_push_all)] | |||
| #![feature(zero_one)] | |||
| #![feature(question_mark)] | |||
| #![feature(env_iterators)] | |||
There was a problem hiding this comment.
Ah this isn't actually required for stable features, so it's ok to leave this off
|
Thanks! Yeah feature annotations aren't required for stable features (which this is), and we generally pick totally arbitrary names for these so |
|
Alright, I have removed the feature, which indeed is not required. The test was made a little less-trusty too. |
|
That all looks good to me, thanks! Could you squash down into one commit as well? |
|
Alright, it's done :) ! |
|
⌛ Testing commit 8f4b7ad with merge 62f0bb1... |
|
💔 Test failed - auto-win-msvc-64-cargotest |
|
I believe to have fixed the function signature to not cause build failures on windows anymore. |
|
💔 Test failed - auto-win-msvc-64-opt-rustbuild |
|
Alright, getting there ! With a little bit of luck, this totally works now. |
|
@alexcrichton Could you have a look again please ? It should go through on windows now. Thank you. |
|
⌛ Testing commit e68a134 with merge 57558d1... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
@alexcrichton The failure seems to be unrelated: Is there anything I can do? Thanks. |
|
@bors: retry nah we just have a lot of network failures recently :( On Sun, Jul 24, 2016 at 11:06 PM, Sebastian Thiel [email protected]
|
…ichton
DoubleEndedIterator for Args
This PR implements the DoubleEndedIterator trait for the `std::env::Args[Os]` structure, as well
as the internal implementations.
It is primarily motivated by me, as I happened to implement a simple `reversor` program many times
now, which so far had to use code like this:
```Rust
for arg in std::env::args().skip(1).collect::<Vec<_>>().iter().rev() {}
```
... even though I would have loved to do this instead:
```Rust
for arg in std::env::args().skip(1).rev() {}
```
The latter is more natural, and I did not find a reason for not implementing it.
After all, on every system, the number of arguments passed to the program are known
at runtime.
To my mind, it follows KISS, and does not try to be smart at all. Also, there are no unit-tests,
primarily as I did not find any existing tests for the `Args` struct either.
The windows implementation is basically a copy-pasted variant of the `next()` method implementation,
and I could imagine sharing most of the code instead. Actually I would be happy if the reviewer would
ask for it.
|
💥 Test timed out |
|
@bors: retry force clean |
|
@bors: retry |
|
⌛ Testing commit e68a134 with merge da8959a... |
|
💔 Test failed - auto-win-gnu-32-opt |
|
@bors: retry On Mon, Jul 25, 2016 at 2:41 PM, bors [email protected] wrote:
|
|
⌛ Testing commit e68a134 with merge 69e335b... |
|
💔 Test failed - auto-win-msvc-64-cargotest |
The number of arguments given to a process is always known, which makes implementing DoubleEndedIterator possible. That way, the Iterator::rev() method becomes usable, among others. Signed-off-by: Sebastian Thiel <[email protected]> Tidy for DoubleEndedIterator I chose to not create a new feature for it, even though technically, this makes me lie about the original availability of the implementation. Verify with @alexchrichton Setup feature flag for new std::env::Args iterators Add test for Args reverse iterator It's somewhat depending on the input of the test program, but made in such a way that should be somewhat flexible to changes to the way it is called. Deduplicate windows ArgsOS code for DEI DEI = DoubleEndedIterator Move env::args().rev() test to run-pass It must be controlling it's arguments for full isolation. Remove superfluous feature name Assert all arguments returned by env::args().rev() Let's be very sure it works as we expect, why take chances. Fix rval of os_string_from_ptr A trait cannot be returned, but only the corresponding object. Deref pointers to actually operate on the argument Put unsafe to correct location
|
@alexcrichton I have placed the unsafe scope into the right place, and rebased everything into one commit which now will hopefully go through. My apologies for all the attempts it took to make work this simple patch. |
…ichton
DoubleEndedIterator for Args
This PR implements the DoubleEndedIterator trait for the `std::env::Args[Os]` structure, as well
as the internal implementations.
It is primarily motivated by me, as I happened to implement a simple `reversor` program many times
now, which so far had to use code like this:
```Rust
for arg in std::env::args().skip(1).collect::<Vec<_>>().iter().rev() {}
```
... even though I would have loved to do this instead:
```Rust
for arg in std::env::args().skip(1).rev() {}
```
The latter is more natural, and I did not find a reason for not implementing it.
After all, on every system, the number of arguments passed to the program are known
at runtime.
To my mind, it follows KISS, and does not try to be smart at all. Also, there are no unit-tests,
primarily as I did not find any existing tests for the `Args` struct either.
The windows implementation is basically a copy-pasted variant of the `next()` method implementation,
and I could imagine sharing most of the code instead. Actually I would be happy if the reviewer would
ask for it.
apasel422
added
the
relnotes
This PR implements the DoubleEndedIterator trait for the
std::env::Args[Os]structure, as wellas the internal implementations.
It is primarily motivated by me, as I happened to implement a simple
reversorprogram many timesnow, which so far had to use code like this:
... even though I would have loved to do this instead:
The latter is more natural, and I did not find a reason for not implementing it.
After all, on every system, the number of arguments passed to the program are known
at runtime.
About the implementation
To my mind, it follows KISS, and does not try to be smart at all. Also, there are no unit-tests,
primarily as I did not find any existing tests for the
Argsstruct either.Critical Comments
The windows implementation is basically a copy-pasted variant of the
next()method implementation,and I could imagine sharing most of the code instead. Actually I would be happy if the reviewer would
ask for it.