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

Constant delegation #81

Merged
merged 1 commit into from
Feb 9, 2025
Merged

Constant delegation #81

merged 1 commit into from
Feb 9, 2025

Conversation

vic1707
Copy link
Contributor

@vic1707 vic1707 commented Feb 7, 2025

fixes: #79

Hi, I think I got it working 😁.
Let me know if anything needs to be changed, if you have other test suggestions or a better documentation !

@vic1707
Copy link
Contributor Author

vic1707 commented Feb 7, 2025

I decided to generate the constant getter for each match arm since it's a const fn and will get optimized out by the compiler.
This helped keep the codegen centralized.

Another approach that could work would be to generate the getter once before the match statement.
If you prefer the later tell me and I'll make the changes 🙃

Copy link
Owner

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks! The implementation looks pretty good. Left a few nits, but otherwise it's close to being ready I think.

I decided to generate the constant getter for each match arm since it's a const fn and will get optimized out by the compiler.

This is fine, we should generate the function locally, so that e.g. this works:

#[test]
fn multiple_consts() {
    trait Foo {
        const A: u32;
        const B: u32;
    }

    struct A;
    impl Foo for A {
        const A: u32 = 0;
        const B: u32 = 0;
    }

    struct Wrapper(A);
    impl Wrapper {
        delegate! {
            to &self.0 {
                #[const(Foo::A)]
                fn a(&self) -> u32;

                #[const(Foo::B)]
                fn b(&self) -> u32;
            }
        }
    }
}

Could you please add the above as a test? Thanks!

@vic1707
Copy link
Contributor Author

vic1707 commented Feb 8, 2025

Thanks for the review, PR updated, let me know if I forgot something (in which case I'm sorry 😅)

Copy link
Owner

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

I think that you forgot an empty comment on one line. Once you remove it, please squash the commits into one and we're good to go :)

src/lib.rs Outdated
@@ -945,6 +990,7 @@ pub fn delegate(tokens: TokenStream) -> TokenStream {
#(#attrs)*
#inline
#visibility #signature {
//
Copy link
Owner

Choose a reason for hiding this comment

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

Forgotten line? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crap it was my marker for where I could inject the constant getter if you asked for it 🤣😅

@Kobzol
Copy link
Owner

Kobzol commented Feb 9, 2025

Reformatting is also needed (see CI).

@vic1707
Copy link
Contributor Author

vic1707 commented Feb 9, 2025

should be good 👍

@Kobzol
Copy link
Owner

Kobzol commented Feb 9, 2025

Thanks! Could you please squash the commits now?

@vic1707 vic1707 force-pushed the constant-delegation branch from eafe967 to 6e96a99 Compare February 9, 2025 13:56
@vic1707
Copy link
Contributor Author

vic1707 commented Feb 9, 2025

done ✅

@vic1707 vic1707 force-pushed the constant-delegation branch from 6e96a99 to 5273fb4 Compare February 9, 2025 15:48
@Kobzol
Copy link
Owner

Kobzol commented Feb 9, 2025

Thank you! :)

@Kobzol Kobzol merged commit f7506ee into Kobzol:main Feb 9, 2025
3 checks passed
@vic1707 vic1707 deleted the constant-delegation branch February 9, 2025 16:52
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.

delegate associated constants ?
2 participants