-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
take on bool
#3189
take on bool
#3189
Changes from all commits
1850366
22f6234
1bee972
e316f5f
2673b66
ef97ace
8b42cd0
730d6f8
d8f615b
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 |
---|---|---|
@@ -0,0 +1,130 @@ | ||
- Feature Name: `take-on-bool` | ||
- Start Date: 2021-10-27 | ||
- RFC PR: [rust-lang/rfcs#3189](https://github.com/rust-lang/rfcs/pull/3189) | ||
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) | ||
|
||
# Summary | ||
|
||
This RFC proposes adding a method to `bool` called `take`, which will save the value of the bool to a variable, set the bool to false, and then return that variable. | ||
|
||
# Motivation | ||
|
||
In many applications, deferred execution of routines is preferable, especially around long standing state. Using flags, or booleans which indicate a condition, is a good way to raise those routines. Taking that flag, while also resetting it, makes this pattern more elegant, without complexifying the `std` inordinately. | ||
|
||
The current one liner to get this same effect is `mem::take`. Adding `mem` into a project often implies a more complex operation than `.take` on a `Copy` tends to be, since `mem::take` (and `mem::replace` in general) is most useful when the type isn't Copy like `bool` is, which allows users to trivially do operations which otherwise require some non-trivial unsafe code. Moreover, in general, the `mem` module has a collection of relatively low-level operations on memory patterns. This makes it a more intimidating toolbox to reach for than a simple method on `bool` could be. | ||
|
||
There are two places where a "Take" pattern is used that is more about API usability, rather than memory handling, but where `mem::take` could be used: `Option` and `bool`. For the former, we already have the `.take` method (and in my Rust experience, anecdotally, is used often). We don't have anything for the latter. This PR's motivation is adding such a method for `bool`. | ||
|
||
# Guide-level explanation | ||
|
||
All .take() does is return the value of a boolean while also setting that value internally to false. It is just like `mem::take`, except it is called as a method instead of a free function. In places where booleans are commonly read and then reset, like dirty flags, this method is useful. | ||
|
||
For example, imagine a common game structure: | ||
|
||
```rs | ||
/// A recursive data structure of positions, commonly used in games to allow a parent/child relationship between transforms. | ||
pub struct SceneNode { | ||
/// a position relative to this Node's parent. | ||
pub local_position: [f32; 2], | ||
/// an authoritative position | ||
pub world_position: [f32; 2], | ||
|
||
pub dirty: bool, | ||
} | ||
|
||
impl SceneNode { | ||
/// We want a new local position! | ||
pub fn set_local_position(&mut self, new_pos: [f32; 2]) { | ||
self.local_position = new_pos; | ||
self.dirty = true; | ||
} | ||
|
||
/// we magically have the parent in this example. | ||
pub fn calculate_world_position(&mut self, parent: &SceneNode) { | ||
// we can take the flag and also unset it in one method call | ||
if self.dirty.take() { | ||
self.world_position = [ | ||
self.local_position[0] + parent.local_position[0], | ||
self.local_position[1] + parent.local_position[1], | ||
]; | ||
} | ||
|
||
/* | ||
// without this RFC, our code would be slightly uglier like this: | ||
if self.dirty { | ||
self.dirty = false; | ||
self.world_position = [ | ||
self.local_position[0] + parent.local_position[0], | ||
self.local_position[1] + parent.local_position[1], | ||
]; | ||
} | ||
*/ | ||
} | ||
} | ||
``` | ||
|
||
# Reference-level explanation | ||
|
||
Implementation should be the following: | ||
|
||
```rs | ||
pub fn take(&mut self) -> bool { | ||
// save the old value to a variable | ||
let val = *self; | ||
// and reset ourselves to false. If we are already false, | ||
// then this doesn't matter. | ||
*self = false; | ||
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. This should perhaps note that using mov al, byte ptr [rdi]
+ and al, 1
mov byte ptr [rdi], 0
ret (Godbolt) 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. Yup -- I didn't want to get too in the weed here, but that's a great point. It mostly goes away once you call the method though 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. That's not a reason to add this method, though, because it's a problem with every single two-variant enum, not just Its root cause is LLVM discarding information about the possible values of the discriminant. rust-lang/rust#85133 (comment) This "just" needs to get fixed, since it's actually hurting substantial amounts of |
||
|
||
val | ||
} | ||
``` | ||
|
||
# Drawbacks | ||
|
||
Save the usual drawbacks of adding a new method to the standard library, there are no drawbacks. | ||
|
||
# Rationale and alternatives | ||
|
||
There are two other possible implementations of the method: | ||
|
||
1. Conditionally branching: | ||
|
||
```rs | ||
pub fn take(&mut self) -> bool { | ||
if *self { | ||
*self = false; | ||
true | ||
} else { | ||
false | ||
} | ||
} | ||
``` | ||
|
||
2. Using `mem::replace` or `mem::take`: | ||
|
||
```rs | ||
pub fn take(&mut self) -> bool { | ||
// or core::mem::take(self) | ||
core::mem::replace(self, false) | ||
} | ||
``` | ||
|
||
In practice, the proposed implementation produces identical code using Godbolt to #2, and the proposed implementation and #2 seem to always produce better code than the #1 alternative above (specifically, they tend to elide jumps more easily). However, in more complex code, these all seem to resolve to more or less the same code, so it's a fairly bike-sheddable difference. | ||
|
||
Alternatives to this method entirely for users are, of course, just writing the code out themselves. Sometimes that may even be preferable for simplicity. | ||
|
||
Users can also use `Option::<()>` instead of `bool` to get this functionality now, for free. Additionally, they could simply wrap `bool` and Deref into it, with the added `.take`. | ||
|
||
This functionality could instead be provided by a crate (e.g. boolinator), but this functionality can be commonly desired and is in keeping with `Option::take`. A crate, however, could provide even more `bool` methods, like `toggle`, which may be useful to some users. | ||
|
||
# Prior art | ||
|
||
Prior art: `AtomicBool::compare_exchange`, which is used for similar purposes. It is a more complex operation, because it allows specifying memory orderings (irrelevant here) and because it can set the boolean to either true or false, or act effectively as a read without modifying. But, it is closely related in that, for example, code being migrated towards or away from Sync support might replace one with the other. | ||
|
||
# Unresolved questions | ||
|
||
None | ||
|
||
# Future possibilities | ||
|
||
We could later add `toggle`. |
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.
As a reader, I prefer this version. It is only one line longer and it is much clearer what is going on.
take
is not immediately obvious unless you are very familiar with the option version, and even then I think you might stop for a minute to consider the semantics (looking just atself.dirty.take()
I might assumedirty
is being treated as some kind of pointer and left asnull
).(I also have a preference against functions which have both an important side effect and return an important value, they tend to be 'too clever' and to surprise readers).