Skip to content

Conversation

@jeehoonkang
Copy link
Contributor

@jeehoonkang jeehoonkang commented Jan 20, 2018

This is a long-waited architectural change that makes crossbeam use subcrates we've been developing so far: crossbeam-epoch, crossbeam-utils, and crossbeam-deque.

It closes #168, closes #157, closes #70, closes #166, closes #165, closes #159, closes #137, closes #116, closes #113, closes #112, closes #60, closes #37, and closes #13. If any of them seems still relevant to the new version, please leave a message here.

It contains a huge API breaking change, so I bumped the version to 0.4.0. Before publishing a new version, I think we need to discuss how to organize this crate and how to document things, esp. those in the subcrates. Maybe we need an RFC for making a consensus.

Cargo.toml Outdated
[features]
nightly = []
default = ["use_std"]
nightly = ["crossbeam-epoch/nightly"]
Copy link
Member

Choose a reason for hiding this comment

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

Could also include crossbeam-utils/nightly and crossbeam-channel/nightly.

Cargo.toml Outdated
nightly = []
default = ["use_std"]
nightly = ["crossbeam-epoch/nightly"]
use_std = ["crossbeam-epoch/use_std"]
Copy link
Member

Choose a reason for hiding this comment

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

Same here, crossbeam-utils/use_std.

[dependencies]
crossbeam-epoch = "0.2"
crossbeam-utils = "0.2"
crossbeam-deque = "0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Do we not want to expose channel for now?

Copy link
Contributor Author

@jeehoonkang jeehoonkang Jan 20, 2018

Choose a reason for hiding this comment

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

Eventually, crossbeam-channel should replace the two queue implementations in sync. I don't know what's the best way to do it. I omitted this migration in this PR because I think it's @stjepang's call to make.

@jeehoonkang
Copy link
Contributor Author

@Vtec234 I adjusted [features] in Cargo.toml as you said. Thanks!

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.

LGTM, at least as a first step in the big refactoring. :)

Just one more thing: let's add crossbeam-channel as a dependency and re-export it as:

#[macro_use]
pub extern crate crossbeam_channel as channel;

In the future, I'd like to see the stack and queues moved into crossbeam-stack and crossbeam-queue, but I guess it's okay if leaving them for now. Also, we should move ArcCell somewhere else (perhaps into crossbeam-atomic?) and clean up crossbeam-utils (it still has an unsound implementation of AtomicOption).

@jeehoonkang
Copy link
Contributor Author

@stjepang hmm, do you want to reexport the macros defined in channel? I think it's not stabilized yet: rust-lang/rust#29638

When I just copy-and-pasted your code snippet in sync/mod.rs, the compiler says:

error[E0468]: an `extern crate` loading macros must be at the crate root
  --> src/sync/mod.rs:11:1
   |
11 | pub extern crate crossbeam_channel as channel;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

When I copy-and-pasted your code snippet in lib.rs, the compiler says:

warning: unused `#[macro_use]` import
  --> src/lib.rs:30:1
   |
30 | #[macro_use]
   | ^^^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default

@ghost
Copy link

ghost commented Jan 26, 2018

Does this solution work?

Also, I think it'd be better to re-export channel inside lib.rs rather than sync/mod.rs.

@jeehoonkang
Copy link
Contributor Author

@stjepang It works, but as a side effect, all the members of crossbeam_channel is now visible in crossbeam, e.g. we have crossbeam::bounded(). I don't think there's a better workaround..

@jeehoonkang
Copy link
Contributor Author

@stjepang The side-effect of visibility (e.g. crossbeam::bounded()) seems too big to my eyes. How about merging this PR without incorporating crossbeam-channel, and then later making another PR for crossbeam-channel?

@ghost
Copy link

ghost commented Feb 4, 2018

Yes, I'm okay with leaving crossbeam-channel out.

@jeehoonkang jeehoonkang merged commit 60103b2 into crossbeam-rs:master Feb 5, 2018
@jeehoonkang jeehoonkang deleted the subcrates branch June 2, 2019 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants