-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #4322: Avoid generating multiple inline accessors for the same ac… #4468
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
Conversation
tests/pos/i4322.scala
Outdated
object Foo { | ||
private[this] var x: Int = 1 | ||
|
||
inline def foo: Int = x + x + x |
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 suggest turning this into a run test and then checking that there's only one accessor method using Class#getMethods
(see https://github.com/lampepfl/dotty/blob/master/tests/run/traitNoInit.scala for an example on how to do something like that)
@@ -83,71 +87,77 @@ object Inliner { | |||
* @param rhs A function that builds the right-hand side of the accessor, | |||
* given a reference to the accessed symbol and any type and | |||
* value arguments the need to be integrated. | |||
* @param seen An map of already generated accessor methods of this kind (getter or setter) |
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.
typo: An map -> A map
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.
Just a couple of improvements using the IdetityHashMap on symbols
@@ -48,6 +48,10 @@ object Inliner { | |||
object addAccessors extends TreeMap { | |||
val accessors = new mutable.ListBuffer[MemberDef] | |||
|
|||
type AccessorMap = mutable.HashMap[Symbol, DefDef] |
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.
Use MutableSymbolMap[T]
@@ -48,6 +48,10 @@ object Inliner { | |||
object addAccessors extends TreeMap { | |||
val accessors = new mutable.ListBuffer[MemberDef] | |||
|
|||
type AccessorMap = mutable.HashMap[Symbol, DefDef] | |||
private val getters = new AccessorMap |
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.
Use newMutableSymbolMap
Even with this PR, we're still generating two accessors for |
scala.annottation.internal.Accessed indicates which symbol is accessed by an inline accessor.
Revise scheme how inline accessors are created so that exactly one inline accessor is created for each member that needs one.
@smarter The new version generates only one accessor per symbol. |
@smarter @nicolasstucki Can one of you give this another review? |
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
accessed.owner.info.decl(accessorName).suchThat(refersToAccessed).symbol | ||
.orElse { | ||
val acc = newAccessorSymbol(accessed, accessorName, accessorInfo) | ||
acc.addAnnotation(Annotation.Accessed(accessed)) |
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 should probably go in newAccessorSymbol
This is a preparation step to enable migrating generation of protected accessors after pickling. The actual migration will be done at a later point. To do this, I refactored much of the logic needed for generating acessors into a new class `AccessProxies`.
Reverted: New annotation on inline accessors (reverted from commit 9cde3dd) Also: Fix accessor definition to work for accesses to members of superclasses.
…cess