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

Seal traits that only intended to be implemented within the crate that defines them #620

Closed
taiki-e opened this issue Dec 24, 2020 · 3 comments · Fixed by #884
Closed

Seal traits that only intended to be implemented within the crate that defines them #620

taiki-e opened this issue Dec 24, 2020 · 3 comments · Fixed by #884

Comments

@taiki-e
Copy link
Member

taiki-e commented Dec 24, 2020

Currently, crossbeam provides the following traits:
https://docs.rs/crossbeam/0.8.0/crossbeam/all.html

Some of these seem intended to be implemented for a particular type and not intended to be implemented by the user.
If I understand correctly, the traits below are not intended to be implemented by the user:

I would prefer to seal these traits using the sealed trait pattern described in API guidelines to prevent them from being accidentally implemented by the user. (Even if we never plan to make breaking changes to these APIs, I believe it's still worthwhile to prevent users from accidentally implementing these traits.)

@taiki-e taiki-e changed the title Seals traits that only intended to be implemented within the crate that defines them Seal traits that only intended to be implemented within the crate that defines them Dec 24, 2020
@taiki-e
Copy link
Member Author

taiki-e commented Dec 24, 2020

If I understand correctly, the traits below are not intended to be implemented by the user:

For other traits:

  • atomic::AtomicConsume
    As the document says, I think this is intended to be implemented for the atomic types of the standard library, but I don't think there is any problem if the user implements this.
    (That said, third-party crates can implement as an inherent method, so I don't think it's a problem to seal it.)

  • epoch::Pointable
    cc @jeehoonkang: Is this trait intended to be implemented by the user? (I guess it's "true", but I'm not clear)

@jeehoonkang
Copy link
Contributor

  • For CompareAndSetOrdering: maybe we can completely switch to compare_exchange and always use two orderings for CAS?
    What do you think?

  • Pointer: I agree that the sealed trait pattern would be nice for this trait.

  • AtomicConsume: I agree that it's "open" to the library users.

  • Pointable: it's intended to be implemented by the users who want to precisely control DST and memory layout stuffs.

@taiki-e
Copy link
Member Author

taiki-e commented Dec 24, 2020

  • For CompareAndSetOrdering: maybe we can completely switch to compare_exchange and always use two orderings for CAS?
    What do you think?

Ah, given that the standard library deprecated the methods that use a single ordering for CAS operation, I think it makes sense to remove CompareAndSetOrdering trait and change compere_and_set to always use two orderings.

EDIT: opened #621 for this.

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

Successfully merging a pull request may close this issue.

2 participants