-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Change isStatic to isStaticOwner in hasLocalInstantiation #19803
Conversation
If you keep the printing you can demonstrate the change in the output, which I think is good evidence:
|
Sounds good; I moved the test to |
object T3 { def t3(): Unit = { class C3; test(classOf[C3]) } } | ||
object T4 { def t4(): Unit = new AnyRef { class C4; test(classOf[C4]) } } | ||
|
||
class T5 { class C5; test(classOf[C5]) } // outer ref currently not elided |
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 think "currently" isn't correct (and thus misleading). Classes that aren't defined in some kind of block are externally instantiable. So they are assumed to be public API, which means a change in the class's implementation can't break the API of that class's constructor.
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 see, this makes things clearer for me, thanks. I will clarify the comment.
But then, we should also not eliminate the outer reference in Got it; no need for an outer pointer in this case; the object can be accessed directly from its path.C1
, should we? C1
is also publicly accessible and instantiable.
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.
Yeah, it's assumed that T1$
is instantiated only once, by val T1, as opposed to multiple T5 instances.
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.
Comments updated.
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.
LGTM, except for 1 comment word 😄
Co-Authored-By: Dale Wijnand <[email protected]> Co-Authored-By: noti0na1 <[email protected]> Co-Authored-By: odersky <[email protected]>
FYI: val Tiker = "tick"
class TickActor extends Actor {
def receive:Receive = {
case Tiker => // Do something
}
} Will generate a constrocutor with
will not. |
Fixes #19569Related to #19569, but not a "fix". It actually allows eliminating outer references in even more cases.