-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #3018: use List instead of Map in InlineLocalObjects #3021
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
Using Map#values resulted in losing the original order or arguments, a List fixes that. The lookup cost will be slightly higher this way, but that should be negligible.
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.
Thanks a lot for the fix. Do we need to make the test run with -optimise
instead of only compile?
case Some(newSym) => ref(newSym) | ||
} | ||
newFieldsMapping.getOrElse(rec.symbol, Nil).collect { | ||
case ((oldSym, newSym)) if oldSym == t.symbol => ref(newSym) |
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 is now linear. Consider using a LinkedHashMap
instead.
// | | New fields, replacements these getters | ||
// | Usages of getters of these classes | ||
// ValDefs of the classes that are being torn apart; = candidates.intersect(gettersCalled) | ||
var newFieldsMapping: Map[Symbol, List[(Symbol, Symbol)]] = null |
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.
Aren't these fields supposed to be private?
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 didn't bother annotating everything private, but yes, everything is private[this]
except inherited methods from transform.localopt.Optimisation
which are private[transform.localopt]
.
LGTM |
@@ -0,0 +1 @@ | |||
-optimise |
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.
flags files are ignored by vulpix, flags have to be put in the code in CompilationTests.scala
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.
fixed in #3029
Using Map#values resulted in losing the original order or arguments, a List fixes that. The lookup cost will be slightly higher this way, but that should be negligible.