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

Remove dependency on syn & quote #16

Closed
jhpratt opened this issue Aug 23, 2020 · 3 comments · Fixed by #20
Closed

Remove dependency on syn & quote #16

jhpratt opened this issue Aug 23, 2020 · 3 comments · Fixed by #20
Labels
C-enhancement Category: A new feature or an improvement for an existing one

Comments

@jhpratt
Copy link

jhpratt commented Aug 23, 2020

Given that this crate has relatively simple behavior, it should be feasible to remove the few proc macro dependencies. Although they are widely used, it's not desirable for every downstream crate to have to rely on them.

I am personally looking for a way to have const fn be enabled automagically for end users of the time crate. With the stabilization of const if/match/loop, a very large portion of the methods are const-capable. The only way to do this without needlessly duplicating every method is with a proc macro — which I presume is why this crate exists. Time is relatively lightweight as it is, and I'd like to keep it that way.

With regard to how this is possible, I am admittedly not experienced at proc macros. However, I believe it would be possible to just search for the first instance of const before the fn keyword, which would always(?) be the proper token. This could then be omitted as necessary for the duplication of the function definition.

After a quick look at this crate's code, it looks like the omission of the quote dependency should be trivial, at the least.

@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label Aug 24, 2020
@taiki-e
Copy link
Owner

taiki-e commented Aug 24, 2020

We certainly need only the information on the const (or fn) and the token that before it. So, we should be able to remove the dependency on syn. (#17 uses syn::parse::Perse, but it should actually be able to replace it with code that parses proc_macro2::TokenStream directly.)

quote is necessary to convert syn or own syntax type into tokens, so I'm not sure if we can remove the dependency on quote, but, as far as I know, quote is very light dependency compared to syn. So I think the benefit of removing quote dependency is not very big.
(FYI: here is the current compile time of #17 (with cargo build --release -Z timings):

const-fn

@jhpratt
Copy link
Author

jhpratt commented Aug 24, 2020

Yeah, quote definitely isn't as big, but given how light this crate is anyways I personally feel it's best to make it even lighter. On my laptop I actually have a proof of concept where I modified the code to work without any dependencies whatsoever. That's obviously not including #17.

Of course, it's your crate, so the decision is ultimately yours to make. This is just my thought as someone who is intested in cutting down build times as much as possible.

bors bot added a commit that referenced this issue Aug 25, 2020
18: Remove dependency on syn r=taiki-e a=taiki-e

a part of #16

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@bors bors bot closed this as completed in 12672ab Aug 25, 2020
@taiki-e
Copy link
Owner

taiki-e commented Aug 25, 2020

Published 0.4.0 which fixes this issue. (dependency on proc-macro2 has also been removed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants