Skip to content
This repository has been archived by the owner on Nov 5, 2018. It is now read-only.

Remove Atomic's _owned methods #35

Merged
1 commit merged into from
Nov 21, 2017
Merged

Remove Atomic's _owned methods #35

1 commit merged into from
Nov 21, 2017

Conversation

jeehoonkang
Copy link
Contributor

Closes #30

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I really like how this panned out. :)

where
O: CompareAndSetOrdering,
P: Pointer<T>,
Copy link

@ghost ghost Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks kinda messy at the moment, but with in-band lifetimes (see here) and impl Trait in argument position (see here), this signature will be simplified to:

pub fn compare_and_set<P: Pointer<T>>(
    &self,
    current: Shared<T>,
    new: P,
    ord: impl CompareAndSetOrdering,
    _: &'g Guard,
) -> Result<Shared<'g, T>, (Shared<'g, T>, P)>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type stands out as really sigil-heavy and could potentially be changed to Result<Shared<'g, T>, CasError<'g, T, P>> with fields like curr and new in CasError or even entirely to CasResult<'g, T, P>. That said, the current interface uses standard types and is understandable without looking up more custom types, so i'm not so sure. The linked RFCs will definitely improve things.

Copy link

@ghost ghost Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of CompareAndSetError<'g, T, P> because it's easy to mistake the tuple (previous, new) for (new, previous).

@ghost ghost merged commit 80c6bae into crossbeam-rs:master Nov 21, 2017
@jeehoonkang jeehoonkang deleted the pointer-trait branch October 31, 2018 15:25
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants