-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Generate Object getter/property macros to remove duplications #15386
base: master
Are you sure you want to change the base?
Generate Object getter/property macros to remove duplications #15386
Conversation
Generates the macros from an external script that will inline the actual macro implementations, instead of using macros to generate the macros and duplicating the actual implementation multiple times.
…l-lang#15386) Generates the macros from an external script that will inline the actual macro implementations, instead of using macros to generate the macros and duplicating the actual implementation multiple times.
60ef33a
to
b3237ec
Compare
{% if name.is_a?(TypeDeclaration) %} | ||
{% var_name = name.var.id %} | ||
{% type = name.type %} | ||
#{@var_prefix}{{var_name}} : {{type}}? {% if name.value %} = {{name.value}} {% end %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG: there are no initial value for the !
variant:
#{@var_prefix}{{var_name}} : {{type}}? {% if name.value %} = {{name.value}} {% end %} | |
#{@var_prefix}{{name}}? |
{% if name.is_a?(TypeDeclaration) %} | ||
{% var_name = name.var.id %} | ||
{% type = name.type %} | ||
#{@var_prefix}{{name}} | ||
{% elsif name.is_a?(Assign) %} | ||
{% var_name = name.target %} | ||
{% type = nil %} | ||
#{@var_prefix}{{name}} | ||
{% else %} | ||
{% var_name = name.id %} | ||
{% type = nil %} | ||
{% end %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is duplicating def_vars
with the sole difference being that we don't check if the block
macro variable is defined because there are none for setter. We can avoid the duplication.
Working on the follow ups: the docs and adding crystal once to protect class getter/property, I realize there is little point for most of the I'm having second thoughts about splitting the docs PR. Both PR are two sides of the same coin: remove the duplication, be it in the documentation of the implementation. Modifying one irremediably ripples to the other. |
What I mean with two sides of the same coin: here I merely moved the documentation and it makes sense to introduce the But as I start writing distinct docs for each macro to outline their distinct behavior, then the But if we try to first change the docs, then we must extract each individual macro, further duplicating the implementations Said differently: 🐔 <=> 🥚 problem. |
Alternative to #15383 that inlines the macros' implementation instead of calling to external macros, but without any impact on compilation times or memory usage.
Like #15383 it avoids the repetition of the implementations across the set of
class_(getter|property)[?]
macros. Since we don't have "macro functions" in Crystal, the "inlining" needs an external crystal script to generate a static file with all the macros.Implementing #14905 will only require to specialize the
Generator#def_getter
method for the class variant, then run the script to regenerate all the macros.