Skip to content

Conversation

@nicolasstucki
Copy link
Contributor

  • Remove dead code
  • Remove uses of scala.Tuple.* match types
  • Remove dynamic prefix from methods in DynamicTuple

@nicolasstucki nicolasstucki self-assigned this Feb 17, 2020
@nicolasstucki
Copy link
Contributor Author

Note that classes in scala.runtime use the lowest abstractions possible (or use .java).

@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

* Remove dead code
* Remove uses of scala.Tuple.* match types
* Remove `dynamic` prefix from methods in `DynamicTuple`
@nicolasstucki nicolasstucki force-pushed the cleanup-runtime-tuple-implemenation branch from 2e05728 to 9aca3a3 Compare February 17, 2020 07:21
@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/8326/ to see the changes.

Benchmarks is based on merging with master (b4e037c)

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/8326/ to see the changes.

Benchmarks is based on merging with master (068c09c)

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM

private def transformTupleCons(tree: tpd.Apply)(implicit ctx: Context): Tree = {
val head :: tail :: Nil = tree.args
defn.tupleTypes(tree.tpe) match {
defn.tupleTypes(tree.tpe.widenTermRefExpr.dealias) match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move .dealias before . widenTermRefExpr? Can we just use widen instead of widenTermRefExpr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the alias appears after the widening of the term ref. I did not find a case where we needed to completely widen the type. For the case of indices and sizes, this might widen the constant typea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only widen the term refs that come from inserting val bindings when inlining.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could mark the parameters an inline to avoid it. But I will remove the inline altogether in a future PR as it does not really have a purpose anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case of indices and sizes, this might widen the constant type.

The type is a tuple type, it thus may not be constant types.

For dealiasing, what I've in mind is type T = a.type. But I guess no programmers will write such code, so it's fine to ignore such cases.

@nicolasstucki nicolasstucki merged commit 19fd9d4 into scala:master Feb 17, 2020
@nicolasstucki nicolasstucki deleted the cleanup-runtime-tuple-implemenation branch February 17, 2020 16:13
@anatoliykmetyuk anatoliykmetyuk added this to the 0.23.0-RC1 milestone Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants