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

feat(ops): op2 support for buffers in async ops #60

Merged
merged 7 commits into from
Jul 22, 2023

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Jul 20, 2023

Supports owned buffers in and out:

#[op2] #[buffer] async fn(#[buffer] input: JsSlice) -> JsSlice {}
#[op2] #[buffer] async fn(#[buffer] input: JsSlice) -> Vec<u8> {}
#[op2] #[buffer] async fn(#[buffer] input: JsSlice) -> Box<[u8]> {}

@mmastrac mmastrac mentioned this pull request Jul 20, 2023
18 tasks
core/runtime/ops.rs Show resolved Hide resolved
Comment on lines +1581 to +1568
#[buffer]
async fn op_async_buffer(#[buffer] input: JsBuffer) -> JsBuffer {
input
}
Copy link
Member

Choose a reason for hiding this comment

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

I know it's late, but... it's somewhat strange that we put #[buffer] before function definition. I'm sure it would be now a lot of work to change, but this would be lovely:

#[op2(async, core)]
async fn op_async_buffer(#[buffer] input: JsBuffer) ->   #[buffer] JsBuffer {
    input
  }

Similarly for serde:

#[op2(core)]
fn op_foo(#[smi] rid: ResourceId) -> #[serde] SomeStruct {}

Copy link
Contributor Author

@mmastrac mmastrac Jul 20, 2023

Choose a reason for hiding this comment

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

I think this is just an unfortunate side-effect of Rust syntax. There are no return-position attributes (I'll double-check).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so there are no attributes supported in the return position. What about this?

#[returns(buffer)] fn op...

Copy link
Member

Choose a reason for hiding this comment

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

Alright, not a big deal. It's more of a nitpick. We can revisit it some time in the future. Let's keep pushing with the current impl

serde_v8/magic/buffer.rs Show resolved Hide resolved
@mmastrac mmastrac changed the title WIP feat(ops): op2 support for buffers in async ops feat(ops): op2 support for buffers in async ops Jul 21, 2023
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM!

@mmastrac mmastrac merged commit dc419bc into denoland:main Jul 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants