-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix #10131: load enum types more lazily #10135
Conversation
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/10135/ to see the changes. Benchmarks is based on merging with master (1ab76c1) |
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
39ccc7d
to
2a4d9f0
Compare
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
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.
Oops, meant to record this review as a "Request changes"
2a4d9f0
to
2d78d27
Compare
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
2d78d27
to
69e7c7b
Compare
var hasError = false | ||
for (i <- 0 until index) | ||
parseAnnotArg(skip) match { | ||
case Some(c) => arr += c | ||
case Some(c: untpd.Tree) => arr += c | ||
case Some(tag: EnumTag) => arr += tag.toTree |
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.
The toTree
here means we're still forcing things early in some cases
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 guess if this works OK with JDK 14 we can merge as is but we should revisit it.
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.
Yes, this is a recursive call, I see no other ways to deal with it elegantly.
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.
We us this call to build up elems
which is used in Some(untpd.JavaSeqLiteral(elems, TypeTree()))
, if we want to delay things properly we'd have to propagate the delay to this construction too: replace EnumTag by Context => untpd.Tree
again, and construct a new Context => untpd.Tree
when any element in the Array is itself a Context => untpd.Tree
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.
Yes, if we go back to Context => untpd.Tree
, things will work, but it will undo the micro-optimizations. I'd say being lazy enough is a good tradeoff. If a problem emerges, we can be lazier (the lazy problem might be in other places as well).
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.
OK to merge if this works with JDK 14, it would be good to add it to the CI, you should be able to do that by reusing this commit: dotty-staging@a40c694 (I didn't make a PR with it at the time because there were other issues with JDK 14 which have been solved now)
69e7c7b
to
8b50107
Compare
Co-Authored-By: Guillaume Martres <smarter@ubuntu.com>
8b50107
to
9dc145d
Compare
Fix #10131: load enum types more lazily
Fix #10133: JDK 14 failure
This is an alternative to #10134 .