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

Expose the Pointer trait as public #74

Merged
merged 3 commits into from
Apr 1, 2018
Merged

Expose the Pointer trait as public #74

merged 3 commits into from
Apr 1, 2018

Conversation

jeehoonkang
Copy link
Contributor

This PR makes the following changes:

  • Mark Pointer as public. This is necessary for the crates that implement data structures which expose Atomic-like API (e.g. store() and compare_and_swap()).

    I'm thinking of concurrent ART as a use case. It's basically the infinite array of Atomic<T>, where initially everything is just Atomic::null(). Basically, I'd like to expose Art::store<P: Pointer<T>>(index: usize, value: P).

  • Add #![deny(missing_docs, warnings, missing_debug_implementations)], which is generally a good thing.

src/lib.rs Outdated
@@ -58,6 +58,8 @@
#![cfg_attr(feature = "nightly", feature(alloc))]
#![cfg_attr(not(test), no_std)]

#![deny(missing_docs, warnings, missing_debug_implementations)]
Copy link

Choose a reason for hiding this comment

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

I don't feel comfortable with denying lints because future compiler updates could suddenly make our code stop compiling. See rust-lang/rust#47204 (comment) for a recent example of a new warning breaking existing crates. This can be especially frustrating for crates depending on crossbeam.

We should be fine with just #![warn(missing_docs, missing_debug_implementations)].

And if that is not enough, we can add RUSTFLAGS="-D warnings" to the Travis config. Better to break our CI builds than the whole ecosystem. :)

Choose a reason for hiding this comment

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

This can be especially frustrating for crates depending on crossbeam.

AFAIA cargo caps lints when building dependencies, so it would only affect crossbeam itself.

@jeehoonkang
Copy link
Contributor Author

@stjepang I couldn't think of that :) I changed it to #![warn(missing_docs, missing_debug_implementations)].

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.

Since we're publicly exposing Pointer, this might be a good time to reconsider the naming choices around Pointer::from_data and Pointer::into_data.

The word data is somewhat vague and could be mistaken for the actual data behind the pointer (of type T). Perhaps from_usize and into_usize would sound better? Any other ideas?

@jeehoonkang
Copy link
Contributor Author

@stjepang I'm happy with from_usize and into_usize. I initially thought from_raw and into_raw, but they probably have a connotation with raw pointers.

@ghost
Copy link

ghost commented Mar 21, 2018

@Vtec234, any thoughts on this rename?

Copy link
Member

@Vtec234 Vtec234 left a comment

Choose a reason for hiding this comment

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

Looks good, and I agree with the rename.

@jeehoonkang
Copy link
Contributor Author

Based on two approvals, I'm merging it. Thanks!

@jeehoonkang jeehoonkang merged commit eed82cc into crossbeam-rs:master Apr 1, 2018
@jeehoonkang jeehoonkang deleted the expose-pointer branch October 31, 2018 15:18
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.

3 participants