-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Perhaps improve the documentation of drain members further #93769
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1628,11 +1628,13 @@ impl String { | |
self.vec.clear() | ||
} | ||
|
||
/// Creates a draining iterator that removes the specified range in the `String` | ||
/// and yields the removed `chars`. | ||
/// Removes the specified range from the string, returning all removed | ||
/// characters as an iterator. | ||
/// | ||
/// Note: The element range is removed even if the iterator is not | ||
/// consumed until the end. | ||
/// # Leaking | ||
/// | ||
/// In case the iterator disappears without getting dropped (using | ||
/// [`core::mem::forget`], for example), the range remains in the string. | ||
Comment on lines
+1636
to
+1637
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t understand why this would need to be specified. It encourages usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my description. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or clarify what you mean with "this": the entire paragraph, or the constraint about what can happen if you leak an instance of this iterator? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this paragraph kind-of says to me "If you want to, during iteration, change your mind and keep the range inside of the original string, feel free to go ahead and just All we should do in these kinds of documentation is say, very explicitly, something along the lines of "please don't ever skip dropping these iterators properly, it is considered a logic error and can, among other things, leave the original structure in an arbitrary unspecified (yet safe-to-use in terms of memory-safety) state with potentially surprising and unexpected behavior when you continue using it afterwards, it can result in arbitrary amounts of additional leaks of the to-be-drained items and/or other items in the same original data structure". I.e. strongly discourage actually leaking the iterators, and perhaps provide a non-exhaustive list of things that can happen upon leaking it anyways, without giving any promises that constrain the set of things that can happen any more than necessary by Rust's memory safety guarantees. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean this following sentence?
If "the history of the notice on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I mean primarily that the description already lists reasons why it would need to be specified as well as reasons not to.
I went over #43244 again and don't see it. It's in the existence of #74652 and its predecessors introducing the paragraph. But I probably misinterperted #73844: it's the description that inspired the clarification, not someone actually using mem::forget and expecting to still look at the vector later. Some other posts too I'm sure, but probably less than I thought. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Intriguing idea, I think no one mentioned that possibility yet.
I agree, but apparently no one wanted to discourage users in the Vec::drain description. |
||
/// | ||
/// # Panics | ||
/// | ||
|
@@ -1652,7 +1654,7 @@ impl String { | |
/// assert_eq!(t, "α is alpha, "); | ||
/// assert_eq!(s, "β is beta"); | ||
/// | ||
/// // A full range clears the string | ||
/// // A full range clears the string, like `clear()` does | ||
/// s.drain(..); | ||
/// assert_eq!(s, ""); | ||
/// ``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind-of prefer the original sentence here. What’s the motivation for the change? The call to
drain
itself does not do anything (yet) beyond creating an iterator. The returned iterator has a lifetime coupled to the mutable borrow; for an operation that “Removes the specified range … returning all removed elements as an iterator” one could expect an iterator that owns the elements and can be created with a shorter mutable borrow not coupled to a lifetime parameter of the type of the iterator.The same applies to
Vec
andString
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the commit for #92902 and should be discussed there or in #92765.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. I didn't realize that there's another unmerged PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I didn't realize anyone would notice this PR this month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just happened to look at the list of latest/new open PRs today, (mainly because I have an open PR myself where I'm a bit impatiently checking whether anything has happened w.r.t. review yet,) then happend to be quickly looking into this one for whatever reason and probably wouldn't even have started any review if I hadn't spotted the typo of "closure". 😅 I hope you don't mind my lengthy side-discussion about validity of collection types that I kind-of opened in this thread 🙈