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

Improved constant values generated code (2) #84

Closed
GilShoshan94 opened this issue Dec 29, 2022 · 4 comments · Fixed by #89
Closed

Improved constant values generated code (2) #84

GilShoshan94 opened this issue Dec 29, 2022 · 4 comments · Fixed by #89

Comments

@GilShoshan94
Copy link
Contributor

Hi @illicitonion and @danielhenrymantilla,

I discovered this crate yesterday and it's seems perfect for me. Thank you !
I am using the last version 0.5.7.
I ran cargo expand on my code to better understand what it is doing.

The result was quite "ugly" (shown bellow).
I looked in the past issues and found #12 that enhances exactly that.
I tested at the 0.4.2 version and indeed the expansion was "pretty".

Then I tried to find at which version the expansion got "ugly" again.
It happended at the 0.5.0 version.

I am looking at the code right now to see how to fix that.
I never wrote macro so it's a bit cryptic to me....

The "ugly" expansion:

impl ::num_enum::TryFromPrimitive for EncodingFormat {
        type Primitive = u8;
        const NAME: &'static str = "EncodingFormat";
        fn try_from_primitive(
            number: Self::Primitive,
        ) -> ::core::result::Result<Self, ::num_enum::TryFromPrimitiveError<Self>> {
            #![allow(non_upper_case_globals)]
            const Postcard__num_enum_0__: u8 = 2;
            const Json__num_enum_0__: u8 = u8::wrapping_add(2, 1);
            const MessagePack__num_enum_0__: u8 = u8::wrapping_add(
                u8::wrapping_add(2, 1),
                1,
            );
            const Pickle__num_enum_0__: u8 = u8::wrapping_add(
                u8::wrapping_add(u8::wrapping_add(2, 1), 1),
                1,
            );
            const BSON__num_enum_0__: u8 = u8::wrapping_add(
                u8::wrapping_add(u8::wrapping_add(u8::wrapping_add(2, 1), 1), 1),
                1,
            );
            const CBOR__num_enum_0__: u8 = u8::wrapping_add(
                u8::wrapping_add(
                    u8::wrapping_add(u8::wrapping_add(u8::wrapping_add(2, 1), 1), 1),
                    1,
                ),
                1,
            );
            const CSV__num_enum_0__: u8 = u8::wrapping_add(
                u8::wrapping_add(
                    u8::wrapping_add(
                        u8::wrapping_add(u8::wrapping_add(u8::wrapping_add(2, 1), 1), 1),
                        1,
                    ),
                    1,
                ),
                1,
            );
            const Bencode__num_enum_0__: u8 = u8::wrapping_add(
                u8::wrapping_add(
                    u8::wrapping_add(
                        u8::wrapping_add(
                            u8::wrapping_add(
                                u8::wrapping_add(u8::wrapping_add(2, 1), 1),
                                1,
                            ),
                            1,
                        ),
                        1,
                    ),
                    1,
                ),
                1,
            );
            #[deny(unreachable_patterns)]
            match number {
                Postcard__num_enum_0__ => ::core::result::Result::Ok(Self::Postcard),
...

The "pretty" expansion:

impl ::num_enum::TryFromPrimitive for EncodingFormat {
        type Primitive = u8;
        const NAME: &'static str = "EncodingFormat";
        fn try_from_primitive(
            number: Self::Primitive,
        ) -> ::core::result::Result<Self, ::num_enum::TryFromPrimitiveError<Self>> {
            #![allow(non_upper_case_globals)]
            const Postcard: u8 = 2;
            const Json: u8 = u8::wrapping_add(Postcard, 1);
            const MessagePack: u8 = u8::wrapping_add(Json, 1);
            const Pickle: u8 = u8::wrapping_add(MessagePack, 1);
            const BSON: u8 = u8::wrapping_add(Pickle, 1);
            const CBOR: u8 = u8::wrapping_add(BSON, 1);
            const CSV: u8 = u8::wrapping_add(CBOR, 1);
            const Bencode: u8 = u8::wrapping_add(CSV, 1);
            match number {
                Postcard => ::core::result::Result::Ok(EncodingFormat::Postcard),
...
@GilShoshan94
Copy link
Contributor Author

I looked at the proc macro code and I don't really get it to be honest... I can follow a bit what's it's going on but cannot edit with confidence...

In the code, from line 393 to line 405

We have:

...
                variants.push(VariantInfo {
                    ident,
                    attr_spans,
                    is_default,
                    is_catch_all,
                    canonical_value,
                    alternative_values,
                });

                next_discriminant = parse_quote! {
                    #repr::wrapping_add(#discriminant, 1)
                };
            }
...

It seems to me that changing #discriminant to #ident would do the trick...

@GilShoshan94
Copy link
Contributor Author

Ok I tried it and it and it first failed because the value got moved into variants (move occurs because ident has type proc_macro2::Ident, which does not implement the Copy trait`)

So I cloned ident and now it worked !

Here the new code:

...
                let ident_clone = ident.clone();
                variants.push(VariantInfo {
                    ident,
                    attr_spans,
                    is_default,
                    is_catch_all,
                    canonical_value,
                    alternative_values,
                });

                next_discriminant = parse_quote! {
                    #repr::wrapping_add(#ident_clone, 1)
                };
            }
...

And this is the "pretty" expansion:

    impl ::num_enum::TryFromPrimitive for EncodingFormat {
        type Primitive = u8;
        const NAME: &'static str = "EncodingFormat";
        fn try_from_primitive(
            number: Self::Primitive,
        ) -> ::core::result::Result<Self, ::num_enum::TryFromPrimitiveError<Self>> {
            #![allow(non_upper_case_globals)]
            const Postcard__num_enum_0__: u8 = 2;
            const Json__num_enum_0__: u8 = u8::wrapping_add(Postcard, 1);
            const MessagePack__num_enum_0__: u8 = u8::wrapping_add(Json, 1);
            const Pickle__num_enum_0__: u8 = u8::wrapping_add(MessagePack, 1);
            const BSON__num_enum_0__: u8 = u8::wrapping_add(Pickle, 1);
            const CBOR__num_enum_0__: u8 = u8::wrapping_add(BSON, 1);
            const CSV__num_enum_0__: u8 = u8::wrapping_add(CBOR, 1);
            const Bencode__num_enum_0__: u8 = u8::wrapping_add(CSV, 1);
            #[deny(unreachable_patterns)]
            match number {
                Postcard__num_enum_0__ => ::core::result::Result::Ok(Self::Postcard),
...

@GilShoshan94
Copy link
Contributor Author

I am by no mean sure if my solution breaks anything. I don't really get the nitty gritty details yet.
Any feedback is welcome.

@GilShoshan94
Copy link
Contributor Author

PR #89 resolves this. Closing the issue.

Also now it's even cleaner as the value are calculated before code generation, so no wrapping_add() even.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant