-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Populate addresses of symbols, types and trees after pickler #416
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
@odersky please review |
To allow other phases to generate their info.
0cbe2fa
to
5597014
Compare
@@ -531,8 +535,16 @@ class TreePickler(pickler: TastyPickler) { | |||
withLength { pickleType(ann.symbol.typeRef); pickleTree(ann.tree) } | |||
} | |||
|
|||
def updateMapWithDeltas[T](mp: collection.mutable.Map[T, Addr]) = { | |||
mp.map{ | |||
case (key, addr) => (key, adjusted(addr)) |
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'd use a simple loop here:
for ((key, addr) <- mp.toList) mp(key) = adjusted(addr)
Shorter, clearer, faster.
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.
Updating a mutable data structure during iteration doesn't work in general. I guess this is safe because you don't change the key set, but I would be cautious about using the same approach in general.
If you were to use an mutable.AnyRefMap
rather than a mutable.Map
(which is a good idea for performance reasons anyway), you can use mp.transformValues(adjusted)
.
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's way I wrote the code this way: at the point when I am updating the datastructure, I'm already iterating over a different one.
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.
@retronym: Good point. We could modify like this
for (key <- mp.keysIterator.toBuffer) mp(key) = adjusted(mp(key))
That looks like the most efficient safe solution. I believe efficiency is a concern here - these maps can become large.
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.
Any reason to avoid AnyRefMap
? IMO that should be the first choice when you need a map in the compiler. Its main claim to fame is that it can calls equals/hashCode
rather than ==
/``##` which is a non-negligible performance win.
Otherwise, LGTM |
21d66aa
to
a1e8c9c
Compare
a1e8c9c
to
e47483a
Compare
Populate addresses of symbols, types and trees after pickler
Backport "Disallow empty parameter clauses in `extension` definition" to 3.3 LTS
To allow other phases to generate additional sections that refer to them.