-
-
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
Add :single module
compile time flag
#15425
base: master
Are you sure you want to change the base?
Add :single module
compile time flag
#15425
Conversation
also uses a specific name to not override the `#single_module?` accessor (used to clone a host compiler for macro runs).
This isn't neded, and it's always overriden by the call to `compiler.release!` a few lines below anyway.
The object now has methods to set cross compiler and add emit targets; they're responsible from enabling the single module property. We thus no longer need the two query methods.
def add_emit_targets(emit_targets) | ||
@emit_targets |= emit_targets | ||
@single_module = true unless @emit_targets.none? | ||
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.
question: I'm wondering why introduce this specific add
method?
The same effect should be achievable with overriding the setter, which keeps the call site identical, and is overall more flexible.
def add_emit_targets(emit_targets) | |
@emit_targets |= emit_targets | |
@single_module = true unless @emit_targets.none? | |
end | |
def emit_targets=(@emit_targets) | |
@single_module = true unless emit_targets.none? | |
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.
To be explicit on the action? The same can be said of #cross_compile!
I guess (itself influenced by #release!
above 🤔
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.
Oh no this is actually fine. If we wanted an elaborate setter, it would actually need to be able to remove the single module flag for full versatility (@single_module = !emit_targets.none?
). But that wouldn't work because @single_module
might've been set by a different flag already.
Then the correct implementation would require to keep the individual components independent and only evaluate the full condition in the query method as in the original commit from this PR.
Just like the
:release
flag it can be useful to know during macros if we're compiling to a single module.For example alongside the
Linkage
annotation, we would be able to mark the__crystal_*
functions as internal only when compiling to a single module. For example:Related to #15426