Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix doc test for Vec::retain(), now passes clippy::eval_order_dependence #81811

Merged
merged 1 commit into from
Feb 13, 2021

Conversation

schteve
Copy link
Contributor

@schteve schteve commented Feb 5, 2021

Doc test for Vec::retain() works correctly but is flagged by clippy::eval_order_dependence. Fix avoids the issue by using an iterator instead of an index.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 5, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 5, 2021

📌 Commit d9d1d3a90992ad0e503b556fad515cd42f87dfbe has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2021
@LingMan
Copy link
Contributor

LingMan commented Feb 6, 2021

That example is supposed to illustrate:

/// The exact order may be useful for tracking external state, like an index.

Now that you removed the index it doesn't make much sense anymore.

@schteve
Copy link
Contributor Author

schteve commented Feb 6, 2021

I see what you mean. I'm not 100% sure what that comment is saying exactly, but maybe changing "index" to "iterator" would be enough to make it right?
/// The exact order may be useful for tracking external state, like an iterator

@LingMan
Copy link
Contributor

LingMan commented Feb 6, 2021

I'm not 100% sure what that comment is saying exactly

Turns out, neither did I. The comment is saying that the result of the ith call of the closure you give to retain will be used to determine whether the ith element of the vec will be retained. That wouldn't be the case if the elements weren't visited in-order. It also says that you can use this property to fetch data from the correct location in other structures - which obviously need to be in sync with the vec you're operating on (keep in this example).

It is NOT talking - as I had misunderstood initially - about the specific binding i which you remove here, but about the high-level concept of indexing into another structure. That high-level operation is still present with the iterator.

You could leave the comment unchanged as it is technically correct after all. Maybe something like
/// The exact order may be useful for accessing externally tracked state.
would be clearer though.

@schteve
Copy link
Contributor Author

schteve commented Feb 6, 2021

After thinking about it a bit I think the main point is that in the first example the decision is made purely based on the element itself, while in the second it is made based on the external state (the array). With that in mind, I reworked the comment as follows:

/// It may be useful to decide which elements to keep based on external state.

@LingMan
Copy link
Contributor

LingMan commented Feb 6, 2021

Sounds good 👍

@schteve
Copy link
Contributor Author

schteve commented Feb 6, 2021

Great! Thanks very much for your input.

@LingMan
Copy link
Contributor

LingMan commented Feb 6, 2021

Hm, didn't notice at first but you replaced The exact order with It. This example is supposed to showcase the exact order property, so I'd leave that first part of the sentence as it was.

/// The exact order may be useful to decide which elements to keep based on external state.

@schteve
Copy link
Contributor Author

schteve commented Feb 6, 2021

Can you point me to somewhere I can read more about the exact order property? I've googled it but it just points back to this same comment. It doesn't mean anything special to me.

@LingMan
Copy link
Contributor

LingMan commented Feb 6, 2021

It refers to the first part of the retain documentation just a few lines above where it says (emphasis mine)

/// This method operates in place, visiting each element exactly once in the
/// original order, and preserves the order of the retained elements.

So for:

let mut vec = vec![1, 2, 3, 4, 5];
let keep = [false, true, true, false, true];
let mut iter = keep.iter();
let f = |_| { *iter.next().unwrap()};

retain will do:

f(&1);
f(&2);
f(&3);
f(&4);
f(&5);

which results in
assert_eq!(vec, [2, 3, 5]);

If the elements weren't visited in the original order, retain could otherwise do this without violating its contract:

f(&2);
f(&1);
f(&5);
f(&4);
f(&3);

giving you
assert_eq!(vec, [1, 3, 5]);

If it wouldn't visit elements exactly once it could even do

f(&1);
f(&1);
f(&2);
f(&2);
f(&3);
f(&3);
...

which would panic on the unwrap.

@schteve
Copy link
Contributor Author

schteve commented Feb 6, 2021

Yeah, ok, I guess it sounded a bit strange the way it's written as "exact order". I found the wording confusing originally, but I get the gist of it. I think I can clarify it though.

/// Because the elements are visited exactly once in the original order, external state may be used to decide which elements to keep.

@schteve
Copy link
Contributor Author

schteve commented Feb 7, 2021

@m-ou-se There were some additional comments after your approval but think it's ready to re-approve now, could you please take a look?

@schteve schteve marked this pull request as draft February 12, 2021 22:56
@schteve schteve marked this pull request as ready for review February 12, 2021 22:56
@schteve
Copy link
Contributor Author

schteve commented Feb 12, 2021

Hi @m-ou-se, are you able to re-approve? Is there some other process for getting this back into the queue?

@m-ou-se
Copy link
Member

m-ou-se commented Feb 13, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 13, 2021

📌 Commit 0488afd has been approved by m-ou-se

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81811 (Fix doc test for Vec::retain(), now passes clippy::eval_order_dependence)
 - rust-lang#81900 (Organize trait test files)
 - rust-lang#81995 (Fix suggestion to introduce explicit lifetime)
 - rust-lang#82031 (Drop an unnecessary intermediate variable)
 - rust-lang#82033 (Refactor `get_word_attr` to return only `Option`)
 - rust-lang#82040 (Add test to prevent src link regression)
 - rust-lang#82041 (Add docs for shared_from_slice From impls)
 - rust-lang#82050 (Added tests to drain an empty vec)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4cb3810 into rust-lang:master Feb 13, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 13, 2021
cuviper added a commit to cuviper/rust that referenced this pull request Jul 28, 2021
The examples added in rust-lang#60396 used a "clever" post-increment hack,
unrelated to the actual point of the examples. That hack was found
[confusing] in the users forum, and rust-lang#81811 already changed the `Vec`
example to use a more direct iterator. This commit changes `String` and
`VecDeque` in the same way for consistency.

[confusing]: https://users.rust-lang.org/t/help-understand-strange-expression/62858
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 30, 2021
…lett

Update the examples in `String` and `VecDeque::retain`

The examples added in rust-lang#60396 used a "clever" post-increment hack,
unrelated to the actual point of the examples. That hack was found
[confusing] in the users forum, and rust-lang#81811 already changed the `Vec`
example to use a more direct iterator. This commit changes `String` and
`VecDeque` in the same way for consistency.

[confusing]: https://users.rust-lang.org/t/help-understand-strange-expression/62858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants