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

Refactor codegen #251

Merged
merged 5 commits into from
Oct 18, 2019
Merged

Refactor codegen #251

merged 5 commits into from
Oct 18, 2019

Conversation

evnu
Copy link
Member

@evnu evnu commented Oct 16, 2019

The proc-macro code generation has a lot of duplicate code over multiple modules. This merge request refactors the modules by pulling common code into the rustler_codegen::context::Context struct and its functions. This is in part as a preparation for #241 in order to simplify de-duplicating the field_atoms.

Copy link
Member

@scrogson scrogson left a comment

Choose a reason for hiding this comment

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

👍

@evnu evnu force-pushed the refactor-codegen branch from 751cb11 to 84f3523 Compare October 17, 2019 07:43
@evnu
Copy link
Member Author

evnu commented Oct 17, 2019

Rebased on master. @filmor do you want to give this PR a glance as well?

@filmor
Copy link
Member

filmor commented Oct 17, 2019

I'll do that tonight, from a quick glance I only think you could drop the repeated pub(crate) in favour of pub in Context to make that one a bit less verbose (enough as the struct is already pub(crate).

rustler_codegen/src/context.rs Outdated Show resolved Hide resolved
rustler_codegen/src/context.rs Outdated Show resolved Hide resolved
@filmor
Copy link
Member

filmor commented Oct 17, 2019

The rest looks good to me.

@evnu evnu force-pushed the refactor-codegen branch from 84f3523 to 5b74eff Compare October 18, 2019 07:43
@evnu evnu merged commit 7a9e37f into rusterlium:master Oct 18, 2019
@evnu evnu deleted the refactor-codegen branch October 18, 2019 07:48
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.

3 participants