-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add ctor proxies before exports #24884
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 ctor proxies before exports #24884
Conversation
odersky
left a comment
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!
|
|
||
| /** Does symbol `sym` need constructor proxies to be generated? */ | ||
| def needsConstructorProxies(sym: Symbol)(using Context): Boolean = | ||
| !sym.isTerm |
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 don't think this optimization is needed. If it is a term, the previous logic would ask isClass and then isType and then return false. Both of these calls are extremely cheap. So, no need to complicate the logic in this case.
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 hasExport(rest) then | ||
| hasExport(rest) && { | ||
| process(rest) | ||
| exported |
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 construct is logically correct but mixes two concerns. I would suggest to something more along the lines of what we had:
if hasExport(rest) then
process(rest)
exportedThere 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 would have made sense as hasExport(rest) && process(rest)
98c2ba1 to
0a80266
Compare
|
Took suggestions (which are discreetly squashed) and rebased. |
Fixes #15944
Fixes #24879
If exports added members, then check for more constructor proxies.
Follow up previous tweak for using constructor proxy of exported member, by preserving its prefix.
Although
needsConstructorProxiesis not expensive, it's also cheap to exclude terms first and save calls; there are many more term members than types.