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

core: Add ResourceTable #3150

Merged
merged 5 commits into from Oct 23, 2019

Conversation

@bartlomieju
Copy link
Contributor

bartlomieju commented Oct 19, 2019

Closes #3134

CC @ry

@bartlomieju bartlomieju force-pushed the bartlomieju:core-resource_table branch from df91327 to bc24532 Oct 19, 2019
bartlomieju added 2 commits Oct 19, 2019
}

/// Abstract type representing resource in Deno.
pub trait Resource: Downcast + Any + Send {

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 19, 2019

Author Contributor

I tried implementing downcasting manually, but got such errors:

error[E0038]: the trait `resources::Resource` cannot be made into an object
  --> core/resources.rs:61:3
   |
61 |   pub fn add(&mut self, resource: Box<dyn Resource>) -> ResourceId {
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `resources::Resource` cannot be made into an object
   |
   = note: method `downcast_ref` has generic type parameters
   = note: method `downcast_mut` has generic type parameters

That's why I used Downcast trait from downcast-rs crate.

)
let fut = futures::future::poll_fn(move || {
let mut table = get_resource_table();
let stream = table.get_mut::<TcpStream>(rid)?;

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 19, 2019

Author Contributor

Would be awesome if we could do something like:

let stream = table.get_mut::<dyn AsyncWrite>(rid)?;
stream.poll_write(&zero_copy_buf)
@ry ry requested a review from piscisaureus Oct 21, 2019
core/resources.rs Outdated Show resolved Hide resolved
core/resources.rs Outdated Show resolved Hide resolved
core/resources.rs Outdated Show resolved Hide resolved
core/resources.rs Show resolved Hide resolved
Copy link
Collaborator

ry left a comment

Looks great! Thank you for this important work!
I defer to @piscisaureus to approve and land.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Oct 21, 2019

Looks great! Thank you for this important work!
I defer to @piscisaureus to approve and land.

Cool, though we shouldn't land it until we figure out how to implement read/write handling

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 21, 2019

Cool, though we shouldn't land it until we figure out how to implement read/write handling

The way you handled it in the example is good.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Oct 21, 2019

Cool, though we shouldn't land it until we figure out how to implement read/write handling

The way you handled it in the example is good.

Can't be translated to CLI though, PTAL at this comment

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 21, 2019

Can't be translated to CLI though, PTAL at this comment

I don't understand why it can't work in the CLI. It's the same operation.

@ry
ry approved these changes Oct 23, 2019
Copy link
Collaborator

ry left a comment

@piscisaureus approves too - thanks @bartlomieju !

@ry ry merged commit 029e833 into denoland:master Oct 23, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2016
Details
test_std windows-2016
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@bartlomieju bartlomieju deleted the bartlomieju:core-resource_table branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.