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

RFC: `core::mem::replace_with` for temporarily moving out of ownership. #1736

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@ticki
Copy link
Contributor

ticki commented Sep 1, 2016

This common pattern deserves a place in the standard library.

Rendered Implementation
RFC: `core::mem::replace_with` for temporarily moving out of ownership.
This common pattern deserves a place in the standard library.
@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 1, 2016

cc. @Sgeo @makoConstruct

# Detailed design
[design]: #detailed-design

A complete implementation can be found [here](https://github.com/rust-lang/rust/pull/36186). This implementation is pretty simple, with only one trick which is crucial to safety: handling unwinding.

This comment has been minimized.

@kennytm

kennytm Sep 1, 2016

Member

Should we just include the code here? The implementation is short, and including makes the RFC self-contained.

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Sep 1, 2016

So, from what I'm reading, panicking inside of the closure results in implementation defined behavior? That seems... bad. I'd rather just define it now to, well, abort is my preference.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 1, 2016

@ubsan Undefined != Unspecified. Check the comments from the code.

Unwinding will make it abort, but the behavior is unspecified, and might thus change in the future (just like the dropping order of a vector's elements and arithmetic overflows).

Edit: @usban edited his comment. Let me just reply to the new edition of it:

Well, the point is to keep all possibilities open, and frankly, there might be even more in the future. One thing we could define is that it mirrors the behavior of destructors in destructors.

@Ixrec

This comment has been minimized.

Copy link
Contributor

Ixrec commented Sep 2, 2016

Speaking of keeping all possibilities open, is there any reason this couldn't be implemented to simply mem::forget the old value to avoid double-drop without aborting or catching the panic? I'm not sure that's what I'd prefer, but it's what I intuitively expected before I saw the abort() in your PR.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 2, 2016

@lxrec forgetting would require true ownership of the inner value or control of the drop flags, neither of which are plausible.

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Sep 2, 2016

@ticki 1) not a male. 2) I am pretty sure I wrote "implementation defined behavior" in my original comment. I don't like implementation defined behavior. And goddamnit, check the gender of the people you're talking about. This is getting ridiculous.

When you say "unspecified", that means "undefined behavior", by the way. A compiler could do anything, unless you actually give it options of what to do. What, exactly, in your RFC is stopping a compiler from writing to null in the case of a panic?

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 2, 2016

@ubsan

  1. I'm very sorry. I shouldn't assume that (damn gendered language. and it's even worse because English isn't my first language).

  2. Not really. The undefined would mean that the compiler could do anything inconsistently or consistenly. Unspecified means that it is not defined, but it is consistent.

What, exactly, in your RFC is stopping a compiler from writing to null in the case of a panic?

The fact that it is safe denotes that it is not breaking invariants (and thus leading to UB).

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Sep 2, 2016

@ticki I've seen a lot of broken safe code :3

My opinion is, obviously, that we should define it. However, I think if we don't, we should give specific options to take, because that is what unspecified is to me. An implementation chooses one of a specific set of options, and doesn't have to document it (unlike implementation defined, where an implementation should document it).

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 3, 2016

I've seen a lot of broken safe code :3

Well, safe code (while it could be broken) can always be assumed to work right. Safe code shouldn't break any invariants.

My opinion is, obviously, that we should define it. However, I think if we don't, we should give specific options to take, because that is what unspecified is to me. An implementation chooses one of a specific set of options, and doesn't have to document it (unlike implementation defined, where an implementation should document it).

There are already a lot of unspecified things in standard library. Vec drop order comes to my mind. Overflows is a case too.

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Sep 3, 2016

@ticki Overflows are defined to be two's complement for signed integers, and reset to zero for unsigned integers OR panic.

Vec drop order (as well as all drop orders, except for scope) are unspecified, but they are given (implicit) options to choose from; forwards or backwards.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 3, 2016

@ubsan

Overflows are defined to be two's complement for signed integers, and reset to zero for unsigned integers OR panic.

Please link. RFC 560 says otherwise.

Vec drop order (as well as all drop orders, except for scope) are unspecified, but they are given (implicit) options to choose from; forwards or backwards.

Well, not really. In theory, any order could be used. In the end, it is unspecified, and having panics unspecified here is neither unsafe nor dangerous. The type system still expresses an important invariant, i.e. that panicking is safe. Whether it aborts, overwrites the value, or anything else is certainly important as well, but the price paid for specifying it is stagnation.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 3, 2016

And, again, unspecified behavior is completely unrelated to undefined behavior.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Sep 3, 2016

C++17 §1.3.26[defns.unspecified]

unspecified behavior

behavior, for a well-formed program construct and correct data, that depends on the implementation.

[Note: The implementation is not required to document which behavior occurs. The range of possible behaviors is usually delineated by this International Standard. — end note ]

Could the RFC just specify some options to choose from (i.e. abort = alternative 1 / catch_unwind = alternative 2 / eat your laundry / etc) and move on? This undefined vs unspecified discussion is really going off-topic.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 3, 2016

Eating your laundry is definitely a guarantee. Haven't you read the Rust website?

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Sep 3, 2016

@ticki the version of RFC 560 you linked is a pre-acceptance draft. The accepted version is here and doesn't leave things unspecified.

Anyway I would agree that a safe function being documented to do something unspecified does not imply that it could do something unsound. In this case I haven't seen any suggestions as to what the code could do besides aborting, so it might stop the argument if we just defined that (I would like to leave the option open to print something if std is available, as I commented on the PR). If not, we could as a compromise adjust it to say "behavior on panic is unspecified, but broken invariants will not be exposed to safe code."

@Sgeo Sgeo referenced this pull request Sep 5, 2016

Closed

New RFCs? #291

[motivation]: #motivation

A common pattern, especially for low-level programmers, is to acquire ownership
of a value behind a reference without immediately giving it a replacement value.

This comment has been minimized.

@joshtriplett

joshtriplett Sep 9, 2016

Member

Can you include a simple example of this pattern and the problem it causes?

`core::mem::replace`, to allow the replacement value being generated by a
closure and even depend on the old value.

Unfortunately, there are no safe implementation in the standard library,

This comment has been minimized.

@joshtriplett

joshtriplett Sep 9, 2016

Member

s/are/is/ to match "implementation". Or alternatively, "the standard library doesn't provide any safe implementation"

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 13, 2016

@ticki

Well, the point is to keep all possibilities open, and frankly, there might be even more in the future. One thing we could define is that it mirrors the behavior of destructors in destructors.

This is probably obvious, but it's early in the morning and I've only had 2 cups of coffee (clearly not enough). Can you say more about the connection to the behavior of unwinding in a dtor? (Do you mean specifically double panic? Or just any dtor in a dtor?)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 13, 2016

I'm generally 👍 on offering this API, I agree it is a very useful primitive. I am wary of the fact that it can abort, which I think is unacceptable for some applications -- though this is currently a bigger problem in Rust, of course, given the "panic during unwinding" behavior (not to mention OOM behavior, stack overflow behavior, and a few other such things). But I can imagine many cases, particularly unsafe code, where it's evident that everything will be fine from inspection.

EDIT: It might be a good idea to be careful with the documentation to appropriately convey the caution that is required here =)

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 13, 2016

@nikomatsakis

This is probably obvious, but it's early in the morning and I've only had 2 cups of coffee (clearly not enough). Can you say more about the connection to the behavior of unwinding in a dtor? (Do you mean specifically double panic? Or just any dtor in a dtor?)

It's a typo. I meant "panic in destructors".

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 13, 2016

@ticki

It's a typo. I meant "panic in destructors".

Let me rephrase. Is there some particular semantics for panicing in destructors (or during unwinding, or both) that would make it possible for you to do something other than abort here?

@plietar

This comment has been minimized.

Copy link

plietar commented Sep 15, 2016

Could this be specialised such that if T: Default, then it gets replaced by the default value on unwind instead of aborting ?
While this goes against the principle that specialisation shouldn't affect externally visible behaviour, it would avoid aborting when possible. Aborting should be the last resort

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Sep 15, 2016

@plietar The point is exactly that there is no substituded value. Indeed, you can use simply mem::replace if you want to replace it.

@bluss

This comment has been minimized.

Copy link

bluss commented Sep 28, 2016

  1. I would be wary of blessing a function that can turn a &mut T into T. Doesn't this extend what you can legally do within the type system? Could I until now presume that if I give a user access to a &mut Token, that they can never come back with an owned Token value, when I gave no method to create one?
  2. The signature of the function must be in the RFC.
@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 4, 2017

The FCP period has closed. While there remain some interesting directions to pursue here, the basic calculus remains: the RFC provides pretty thin motivation, and its detailed design suffers from drawbacks that might be addressable with further iteration. It'd be good to continue experimenting in the crates.io ecosystem, and come back with a fresh RFC (which summarizes the key points from this one) if there's a substantially new design or new motivation.

Thanks for the RFC, and thanks everyone for the great discussion!

@aturon aturon closed this Jan 4, 2017

@Ericson2314 Ericson2314 referenced this pull request Jan 6, 2017

Closed

Stackless coroutines #1823

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Mar 10, 2017

Just to provide some additional motivation: I stumbled on this RFC just now mostly by accident, and I realized I had previously written code that made this exact mistake (i.e. didn't abort on panics in the constructor).

@lostman

This comment has been minimized.

Copy link

lostman commented Apr 10, 2017

I wrote a similar function (ignoring safety concerns) recently when implementing a simple list:

enum List<T> {
    Nil,
    Cons(T, Box<List<T>>)
}

In particular I wanted to implement IntoIter:

struct IntoIter<T>(List<T>);

impl<T> IntoIterator for List<T> {
    type Item = T;
    type IntoIter = IntoIter<T>;
    fn into_iter(self) -> Self::IntoIter {
        IntoIter(self)
    }
}

My first implementation was:

impl<T> Iterator for IntoIter<T> {
    type Item = T;
    fn next(&mut self) -> Option<T> {
        match mem::replace(&mut self.0, List::Nil) {
            List::Nil => None,
            List::Cons(x, l) => {
                mem::replace(&mut self.0, *l);
                Some(x)
            }
        }
    }
}

Which seems quite bad. First, it requires replacing the list with Nil. What if I didn't have a value that I could use? Or if the empty was expensive? Second, doing replace twice requires copying values 6 times which; isn't this inefficient?

Using replace_with:

pub fn replace_with<T,R,F>(dest: &mut T, f: F) -> R
    where F: Fn(T) -> (R, T)
{
    unsafe {
        let (r, v) = f(std::ptr::read(&*dest));
        std::ptr::write(dest, v);
        r
    }
}

Note that f in this version returns a tuple so that the return value of replace_with can be different than the value written to dest. This is a very convenient pattern I'm used to from Haskell's atomicModifyIORef.

impl<T> Iterator for IntoIter<T> {
    type Item = T;
    fn next(&mut self) -> Option<T> {
        replace_with(&mut self.0, |l| {
            match l {
                List::Nil => (None, List::Nil),
                List::Cons(x, rest) => (Some(x), *rest)
            }
        })
    }
}

I'm new to Rust but unless I'm missing something replace_with seems essential. How else would one deal with recursively defined values? Deconstruct a value, do something with it and then move to a sub-value is a very common pattern. Doing that safely and mutably (hence efficiently) would be a big win.

@bchallenor

This comment has been minimized.

Copy link

bchallenor commented Jan 21, 2018

Given that this was closed for lack of motivating examples, I'd like to link to this thread where I bumped into this issue. I was also tempted to implement it myself, and didn't consider panics.

@bergus

This comment has been minimized.

Copy link

bergus commented Jun 10, 2018

@antrik Regarding the name, how about replace_through(f)?

@briansmith The double-core::mem::replace-pattern is nice when you a) have a value that makes sense to be left in case of a panic b) can construct that value. However, in state machines it seems to be quite common that the value to be replaced is non-copyable and can only be obtained through the API that is supposed to make the new value from the old one.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Jun 11, 2018

@bergus Yes, I think everybody here understands that. The solution is to make abort-on-panic the one and only panic semantics. Then we'd never need poisoning anywhere, including in replace_with or in Mutex.

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jul 10, 2018

To echo @bergus' comment, I recently ran into this with the state machine pattern of transitioning between states by taking something by value, and replace_with would have been perfect. In effect, I was writing:

struct StateA { ... }
struct StateB { ... }
struct StateC { ... }

impl StateA {
    // note: takes self by value
    fn transition_b(self) -> State B { ... }
}

enum State {
    A(StateA),
    B(StateB),
    C(StateC),
}

struct Foo {
    // how do I transition this field between states?
    state: State,
}
@alecmocatta

This comment has been minimized.

Copy link

alecmocatta commented Nov 8, 2018

Just to add to the 👍 s, I bump into this every couple of months, and I too have formerly relied on a non-panic-safe hack involving ptr::{read,write}. I stumbled upon this RFC by literally googling for "rust replace_with".

Here's my crate that resolves the common cases for me: replace_with.

fn replace_with<T, D: FnOnce() -> T, F: FnOnce(T) -> T>(dest: &mut T, default: D, f: F);
fn replace_with_or_abort<T, F: FnOnce(T) -> T>(dest: &mut T, f: F);
fn replace_with_or_default<T: Default, F: FnOnce(T) -> T>(dest: &mut T, f: F);

Implemented using a destructor rather than std::panic::catch_unwind() as @alexcrichton's implementation above.

@vi

This comment has been minimized.

Copy link

vi commented Nov 8, 2018

@alecmocatta

This comment has been minimized.

Copy link

alecmocatta commented Nov 8, 2018

@vi Thanks, it differs from take_mut in a couple of ways:

  • using a destructor vs panic::catch_unwind() to avoid the optimisation-blocking call to __rust_maybe_catch_panic;
  • the API design: the un-suffixed replace_with() accepts a closure for the user to produce a T in case of panic, rather than take_mut::take()'s surprising abort. For the common case of wanting the abort, it is contained to a function with _or_abort in its name to make explicit its dangerousness.

Both minor but, for me at least, both important.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 8, 2018

  • the un-suffixed replace_with() accepts a closure for the user to produce a T in case of panic, rather than take_mut::take()'s surprising abort.

If/when that closure panics, there would still be a surprising abort, right?

@alecmocatta

This comment has been minimized.

Copy link

alecmocatta commented Nov 8, 2018

@briansmith Indeed, though note in that situation the user's code would have to have panicked twice, once in the f() closure and once in the default() closure. Aborting after two consecutive panics is (to me) less surprising than aborting on first panic.

I look forward to when this whole concept is rendered redundant by the type system allowing temporary moves out of &mut T, but until then my aforementioned crate is the best solution I have to a pain point that's been nagging me for years!

@Sgeo

This comment has been minimized.

Copy link

Sgeo commented Nov 9, 2018

@alecmocatta That's funny, take_mut originally used a destructor, and switched to catch_unwind based on people's recommendations, although I guess I didn't fully evaluate those recommendations before implementing.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 9, 2018

@alexcameron89 I do not think @briansmith was talking about the default() call. The point is that if f panics (once), the only sound choice is to abort the program. If you just panic, other threads could observe the bad value

Think of something like

fn foo(x: &mut Vec<i32>) {
  scoped_thread_spawn(|| {
    mem::replace_with(x, |vec| { drop(vec); panic!() });
  });
  println!("{:?}", x);
}

With your crate, this does a use-after-free.

@alecmocatta

This comment has been minimized.

Copy link

alecmocatta commented Nov 9, 2018

@RalfJung "The point is that if f panics (once), the only sound choice is to abort the program." My crate, like take_mut::take_or_recover and the various approaches discussed above, enforce that a valid value is written back in case of panic. With my fn replace_with<T, D: FnOnce() -> T, F: FnOnce(T) -> T>(dest: &mut T, default: D, f: F), this is done by running the default closure while panicking, and writing the resulting T back to dest. If default also panics, then the process aborts.

I think @briansmith's point is whether aborting on the panic of both default and f is any less surprising than aborting on the panic of f as per take_mut::take and the original RFC's replace_with. I think it is – I agree with what @alexcrichton notes above: "the two-closure approach avoids aborts in the "standard case" where the mapping closure (FnOnce(T) -> T) panics. If the second closure panics, however, then it does indeed abort. That's somewhat more justifiable though as it's documented as only being called during a panic, so that way users know to take care in what it calls."

For the avoidance of talking at cross-purposes, here's the implementation:

struct CatchUnwind<F: FnOnce()>(Option<F>);
impl<F: FnOnce()> Drop for CatchUnwind<F> {
	fn drop(&mut self) {
		self.0.take().unwrap()();
	}
}

fn catch_unwind<F: FnOnce() -> T, T, P: FnOnce()>(f: F, p: P) -> T {
	let mut x = CatchUnwind(Some(p));
	let t = f();
	let _ = x.0.take().unwrap();
	mem::forget(x);
	t
}

pub fn replace_with<T, D: FnOnce() -> T, F: FnOnce(T) -> T>(dest: &mut T, default: D, f: F) {
	unsafe {
		let t = ptr::read(dest);
		let t = catch_unwind(move || f(t), || ptr::write(dest, default()));
		ptr::write(dest, t);
	}
}
pub fn replace_with_or_default<T: Default, F: FnOnce(T) -> T>(dest: &mut T, f: F) {
	replace_with(dest, T::default, f);
}
pub fn replace_with_or_abort<T, F: FnOnce(T) -> T>(dest: &mut T, f: F) {
	replace_with(dest, || process::abort(), f);
}

And an example:

enum States {
	A(String),
	B(String),
}
impl States {
	fn poll(&mut self) {
		replace_with(
			self,
			|| States::A(String::new()),
			|self_| match self_ {
				States::A(a) => States::B(a),
				States::B(a) => States::A(a),
			},
		);
	}
}
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 9, 2018

@alecmocatta oops I missed that D closure, I should read more carefully.

@alecmocatta

This comment has been minimized.

Copy link

alecmocatta commented Nov 9, 2018

@RalfJung No worries I suspected there was a crossed wire somewhere!

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 10, 2018

I think @briansmith's point is whether aborting on the panic of both default and f is any less surprising than aborting on the panic of f as per take_mut::take and the original RFC's replace_with.

I think it's more about naming. We have replace_with() and replace_with_or_abort(). IMO this is somewhat misleading about whether replace_with() ever aborts. Maybe it would be better to replace things like so:

enum Fallback<D, T> {
    Const(T),
    AbortOnPanic(FnOnce() -> T)
}

replace_with(x, y, z) -> replace_with(x, Fallback::AbortOnPanic(y), z)
replace_with_or_abort(x, y, z) -> replace_with(x, Fallback::AbortOnPanic(|| unreachable!()), z)
replace_with_or_default(x, y) -> replace_with(x, Fallback::Const(Default::default()), z)

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 11, 2018

In a private codebase, I tried out the API I suggested in my previous comment. Now I realize I was wrong to suggest that replace_with_or_default should be replaced with my new proposed replace_with function. I think the functionality of replace_with_or_default() should be a separate function. However, I do think there's no reason to restrict it to implementations of Default.

Further, I think it would be useful to to be consistent with the naming convention used by functions like Result::or (accepts an alternate value) and Result::or_else (takes a closure).

pub struct AbortOnPanic<T, D: FnOnce() -> T>(pub D);

impl<T, D: FnOnce() -> T> AbortOnPanic<T, D> {
    #[inline]
    pub fn abort_on_panic(self) -> T { (self.0)() }
}

pub fn replace_with<T, F: FnOnce(T) -> T>(dest: &mut T, fallback: T, f: F) { ... }

pub fn replace_with_or_else<T, D: FnOnce() -> T, F: FnOnce(T) -> T>(
    dest: &mut T, 
    fallback: AbortOnPanic<T, D>, 
    f: F
) { ... }

This would give the following mapping:

replace_with(x, y, z) -> replace_with_or_else(x, replace_with::AbortOnPanic(y), z)
replace_with_or_abort(x, y, z) -> replace_with_or_else(x, replace_with::AbortOnPanic(|| unreachable!()), z)
replace_with_or_default(x, y) -> replace_with(x, Default::default(), z)

I noticed the proposed implementation uses ptr::read() and ptr::write(). I think a variant with read_volatile()/write_volatile() would be warranted too. Similarly, I think variants that do atomic reads/writes are needed, for atomic types.

In general, it isn't clear if/how to make this kind of API work for a type T that has interior mutability or how to prevent it from being used with such a type.

[Edited to fix confusing typo.]

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 11, 2018

I think a variant with read_volatile()/write_volatile() would be warranted too
I think variants that do atomic reads/writes are needed, for atomic types.

I am fairly sure these are not needed: We are taking the input in a mutable reference. This excludes all aliases. &mut AtomicUsize is a unique pointer and we can always do non-atomic accesses to such a location. There even is a safe way to do that. Similarily, &mut is a pointer marked as dereferencable, which permits LLVM to insert spurious loads. So using &mut in situations where volatile would be needed is incorrect.

Your concerns about ptr::read and ptr::write would apply if we were talking about &T, but of course this entire construction is about &mut T only. Interior mutability has no "effect" on mutable references.

@alecmocatta

This comment has been minimized.

Copy link

alecmocatta commented Nov 12, 2018

@briansmith Your

replace_with(dest, Default::default(), f);
// and
replace_with_or_else(dest, Fallback::Const(y), f)

are (unless I'm misunderstanding you, please correct me if I am!) a) essentially equivalent and b) equivalent to

*dest = f(mem::replace(dest, Default::default()));
// and
*dest = f(mem::replace(dest, y));

respectively. (Equivalent, excluding contents of x on panic as viewed in a debugger, but I can't think of a compelling reason why that would matter.) Personally I'd be keen to avoid this redundancy, do you have any thoughts given this?

I also agree with @RalfJung that a volatile version isn't necessitated, and that interior mutability isn't a concern as we have &mut T rather than &T.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 12, 2018

@alecmocatta I'd say that @briansmith's functions are much easier to read, much clearer in their intent than your constructions involving mem::replace.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 12, 2018

a) essentially equivalent and b) equivalent to

@alecmocatta It is true that if there is a default/poison/sentinal value of the type, then you can just use core::mem::replace twice to get the effect of replace_with. This was explained many, many comments ago (this is a long thread). The advantage of replace_with in this situation is that it saves one core::mem::replace in the common situation where there are no panics, without the compiler having to do complex control flow analysis to optimize away the first mem::replace.

Thanks for pointing out that interior mutability is not an issue with the current API.

Regarding atomics, I guess AtomicUsize::get_mut() and related things are based on the assumption that it is OK to use non-atomic stores to write an atomic value. I don't know if that's always true, but it seems like it's been resolved already since otherwise get_mut() wouldn't exist. I was thinking it might be useful to have a version of this that is optimized for atomic operations, but usually the initial store of the sentinel value is needed for protocols built on top of atomics, so I can't think of any use case for an atomics-optimized variant.

Regarding volatile, I understand the point that we can assume that the value isn't a volatile object because we have a reference to it and it's not safe to have a reference (vs. a pointer) to a volatile object in Rust. However, my point is different; the idea of avoiding storing a sentinel with core::mem::replace as an optimization seems just as useful (maybe more useful) for volatile objects, so it might make sense to have a variant of replace_with and replace_with_or_else that work for volatile objects, with a different type signature and a different name.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 13, 2018

I guess AtomicUsize::get_mut() and related things are based on the assumption that it is OK to use non-atomic stores to write an atomic value. I don't know if that's always true, but it seems like it's been resolved already since otherwise get_mut() wouldn't exist.

It is mandated by the C standard. Whatever that means, but it likely does mean that even obscure hardware architectures support it.

my point is different; the idea of avoiding storing a sentinel with core::mem::replace as an optimization seems just as useful (maybe more useful) for volatile objects, so it might make sense to have a variant of replace_with and replace_with_or_else that work for volatile objects, with a different type signature and a different name.

I see. Yes, that makes sense. I'd generally be cautious around using anything even resembling a high-level interface for volatile, because after all you do need to control exactly which reads and writes are happening. replace_with also does not strike me as something that comes up a lot in writing device drivers, but then I do not know much about writing device drivers so I probably should not make such assumptions. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment