-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Move quoted null and unit encoding to internal #9551
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
Move quoted null and unit encoding to internal #9551
Conversation
These are opimizations for `'{}`/`'{()}` and `'{null}` which should not be used directly
518c51a
to
566bdf5
Compare
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.
Otherwise, LGTM
def Unit: QuoteContext ?=> quoted.Expr[Unit] = qctx ?=> { | ||
import qctx.tasty._ | ||
Literal(Constant(())).seal.asInstanceOf[quoted.Expr[Unit]] | ||
} |
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.
It seems both can be val
instead of def
?
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.
They could but they would not get the optimization where the context parameter is passed directly as one of the method parameters and the closure is not created. We effectively call the following method at runtime (not closure creation needed):
def Unit(using q: QuoteContext): quoted.Expr[Unit] = {
import qctx.tasty._
Literal(Constant(())).seal.asInstanceOf[quoted.Expr[Unit]]
}
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.
Good point, maybe we can write it directly, instead of the original form?
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.
It is like this to align with the quote internal encoding to simplify the transformation.
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.
I will consider changing the encoding but this is a larger change that will take some time to accomplish.
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.
Thanks for the clarification, it makes sense.
No description provided.