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

Proc macro params optimizations and tests. #421

Merged
merged 13 commits into from Jul 27, 2021
Merged

Conversation

maciejhirsz
Copy link
Contributor

Fixes allocation in proc macro param parsing (#403). Closes #404.

}
}

pub fn traverse_type(ty: &mut Type, replaced: &mut bool, f: fn(&mut Type, &mut bool)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this separation; def makes the code cleaner!

@@ -47,6 +48,16 @@ mod rpc_impl {

#[subscription(name = "echo", unsub = "no_more_echo", item = u32)]
fn sub_with_params(&self, val: u32);

#[method(name = "params")]
fn params(&self, a: u8, b: &str) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I reckon it'd be nice to have some more tests to exercise this lisetime stuff (if that makes sense); like, trying a couple of custom types with generics (elided or not) and a type with more than one lifetime to check that we hit a compile error (and preferably a nice one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a bunch of tests for that purpose :)

@jsdw
Copy link
Collaborator

jsdw commented Jul 21, 2021

Looks awesome! If I understand right, a big change in this PR is actually adding support for lifetimes when using the RPC macro? A couple of questions came up (assuming that's true) around adding more tests to exercise this lifetime stuff, and throwing a friendly error when more than one lifetime is encountered.

Copy link
Collaborator

@jsdw jsdw 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 to me!

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Logic, and syntax lgtm. Not super familiar with the bigger picture here, but overall no grumbles from me.

@maciejhirsz maciejhirsz merged commit 3d52c6a into master Jul 27, 2021
@maciejhirsz maciejhirsz deleted the mh-proc-macro-opt branch July 27, 2021 07:44
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.

Support direct RPC calls
3 participants