-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Avoid creating NumericRange instances when not needed #3289
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
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3289/ to see the changes. Benchmarks is based on merging with master (badc855) |
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
| val argParams = | ||
| for (i <- List.range(1, arity + 1)) yield | ||
| enterTypeParam(cls, paramNamePrefix ++ "T" ++ i.toString, Contravariant, decls) | ||
| val argParams = List.tabulate(arity) { i => |
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.
Rename to argParamRefs?
| val startOffset = source.offsetToLine(start) | ||
| val endOffset = source.offsetToLine(end + 1) | ||
| if (startOffset >= endOffset) line :: Nil | ||
| else List.tabulate(endOffset - startOffset)(i => i + startOffset) |
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.
Removing the offset to add it back is confusing, I would just keep the original code here since it shouldn't be performance-sensitive, or do (startOffset to endOffset).toList maybe.
Initializing NumericRange instances is expensive due to the implicit class tags used.