-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Attempt to inline by name matcher methods #19631
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
final class ByName1(val x: Int) extends AnyVal: | ||
inline def isEmpty: Boolean = x == 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not believe we have a way to make this work. The JVM would probably inline these calls anyway. |
||
inline def get: String = x.toString | ||
|
||
object ByName1: | ||
inline def unapply(x: Int): ByName1 = new ByName1(x) | ||
|
||
def useByName1PatMatch: String = | ||
val x = 1 | ||
x match | ||
case ByName1(s) => s | ||
|
||
final class ByName2(val x: Int) extends AnyVal: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case actually produces valid scala code with |
||
inline def isEmpty: false = false | ||
inline def get: Int = x | ||
|
||
object ByName2: | ||
inline def unapply(x: Int): ByName2 = new ByName2(x) | ||
|
||
def useByName2PatMatch: Int = | ||
val x = 1 | ||
x match | ||
case ByName2(s) => s | ||
|
||
final class Accessor(val x: Int) extends AnyVal: | ||
inline def _1: Int = x + 1 | ||
inline def _2: String = x.toString | ||
|
||
final class ByName3(val x: Int) extends AnyVal: | ||
inline def isEmpty: Boolean = x == 1 | ||
inline def get: Accessor = new Accessor(x) | ||
|
||
object ByName3: | ||
inline def unapply(x: Int): ByName3 = new ByName3(x) | ||
|
||
def useByName3PatMatch: (Int, String) = | ||
val x = 1 | ||
x match | ||
case ByName3(i, s) => (i, s) |
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.
This will not work. Everything must be inlined after this stage.
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.
Could we inline where the calls are introduced then i.e. during the pattern matcher?
Or do we need to do something like the trick that has been done with the unapply body currently and reduced in the
InlinePatterns
stage?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.
That can be problematic. The code that is inlined assumes that we have not been transformed by the phases that come after inlining (and before pattern matching).
That trick will not work well for name-based pattern matching.
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.
If you really need to remove the code that you generate in #19577 we might need another way. Maybe a possible approach is to change the pattern matcher to do something specific for methods on records.
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.
Am I right in thinking that it would require a change to the spec? Would there be appetite to add something Java-specific to the spec? It feels quite unsatisfying to need to have a Java-specific clause in the spec to be honest.
It might be worth just eating the cost of the allocation and boxing/unboxing to be honest - as you said, I expect the JIT to inline it anyway.
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's a spec change anyway. Whether because you're synthesizing stuff that's not there, or because you change pattern matching logic.
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 did not realise that - in which case let me investigate the pattern matcher changes required and put together a proposal.