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

Sibling enum conversions #22

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Nexelous
Copy link
Contributor

Fixes #16.

If a subenum, A, is a subset of another subenum, B, From<A> for B is generated, otherwise TryFrom<A> for B is generated and uses the ConvertError from the child to parent conversions.

@paholg
Copy link
Owner

paholg commented Sep 19, 2023

This is great!

I'm a bit worried about how much code could be generated if someone has a lot of subenums, though. Maybe it would make sense to make this opt-in?

Something like this maybe:

#[subenum(EnumB(from(EnumC)), EnumC(try_from(EnumB))]
enum EnumA {
    #[subenum(EnumB, EnumC)]
    A,
    #[subenum(EnumB)]
    B,
}

Thoughts?

@Nexelous
Copy link
Contributor Author

Yea, you're right, it should probably be opt-in. But the example you gave starts conflicting with subenum-specific macros; you would have to parse those 2 attributes and then pass the rest as normal attributes. Also, it puts the responsibility of deciding whether From or TryFrom should be used on the user.

I don't know how the first point can be solved (or if it is even that big of an issue), but for the second point, maybe a more generalized name like to_sibling instead of from and try_from could work.

@paholg
Copy link
Owner

paholg commented Sep 19, 2023

Fair! Though I'd prefer from_sibling, to match the general Rust pattern of implementing from not into.

@itotallyrock
Copy link
Contributor

itotallyrock commented Sep 24, 2023

Not sure if we're trying to discuss library design here, so let me know if this should be on the original issue as an rfc or sorts.

Regardless....

it should probably be opt-in

While it would be beneficial from a compiler performance perspective, in terms of generated code, I think we should consider whatever requires the least boilerplate for the most general use case.

Don't make the majority of users pay for niche situations they aren't likely to find themselves in.

If the performance hit is too significant, then I understand making it opt in (but I'm unaware how to best benchmark the compiler for a proc macro).
However, I'd suspect most developers who use this library aren't really concerned with compiler performance.
Not saying we should ignore it, but if we're going to increase verbosity we should at least know it was justified through benchmarks.

Also, we don't necessarily have to make it opt-in within "code".
I believe a cargo feature, perhaps included in default, could be more reasonable than complicating the macro's syntax or using an auxiliary proc macro.

@paholg
Copy link
Owner

paholg commented Sep 24, 2023

A cargo feature is not the right way to handle this. If a dependency opts in, that would opt you in as well. If it's going to be opt-in, it really should be per-use.

Perhaps it's fine, even for large enums with lots of siblings, but I would want to see those benchmarks.

One thing about making it opt-in is you can always remove that requirement later; it's much harder to do the opposite.

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 Conversion Between Subenums Without Converting to Parent
3 participants