SI-9319 Avoid "filename too long" errors for anon functions#4505
Closed
retronym wants to merge 2 commits intoscala:2.11.xfrom
Closed
SI-9319 Avoid "filename too long" errors for anon functions#4505retronym wants to merge 2 commits intoscala:2.11.xfrom
retronym wants to merge 2 commits intoscala:2.11.xfrom
Conversation
Member
Author
|
Review by @lrytz |
80dab2c to
e4c0ae4
Compare
Member
|
LGTM. For the failing tests,
|
Under -Ydelambdafy:method, the name of the anonymous class is derived from the names of the original enclosing class and term of the function. However no effort is made to conform to the `-Xmaxclassname` setting when constructing the class name from the components. Recently, in 029cce7, I changed uncurry to selectively fallback to the old method of emitting lambdas when we detect that `-Ydelambdafy:method` would subvert specialization. As a result of this, we could now have a delambdafy translated function enclosed in an anonymous class representing an old-style function. If that function, in turn, is heavily nested, its long name now contributes a long suffix to the delambdafy class's name. This led to a "filename too long" error in `run/kmpSliceSearch.scala`. I have extract the essence of the failure in `pos/t9319.scala`. I then changed delambdafy to use the unexpanded names of the owners rather than the full name. This makes things work as expected. I haven't further protected against pathological cases, given that in 2.12.x and indylambda, we won't ever emit these classes. A test for the names of delambdafy functions was modified to account for this change.
Recently, in 029cce7, I changed uncurry to selectively fallback to the old method of emitting lambdas when we detect that `-Ydelambdafy:method`. The change in classfile names breaks the expectations of the test `innerClassAttribute`. This commit changes that test to avoid using specialized functions, so that under -Ydelambdafy:method all functions are uniform. This changes a few fresh suffixes for anonymous class names under both `-Ydelambdafy:{inline,method}`, so the expectations have been duly updated. Similarly, I have changed `javaReflection` in the same manner. Its checkfiles remained unchanged.
Member
Author
|
Let's discuss this one. Maybe on second thoughts we should just leave this broken and force the test to |
This was referenced May 26, 2015
lrytz
added a commit
to lrytz/scala
that referenced
this pull request
May 28, 2015
See discussion on scala#4505. The issue will go away when moving to indy-lambda.
lrytz
added a commit
to lrytz/scala
that referenced
this pull request
May 28, 2015
There were two issues in the new inliner that would cause a VerifyError and an IllegalAccessError. First, an access to a public member of package protected class C can only be inlined if the destination class can access C. This is tested by t7582b. Second, an access to a protected member requires the receiver object to be a subtype of the class where the instruction is located. So when inlining such an access, we need to know the type of the receiver object - which we don't have. Therefore we don't inline in this case for now. This can be fixed once we have a type propagation analyis. https://github.com/scala-opt/scala/issues/13. This case is tested by t2106. Force kmpSliceSearch test to delambdafy:inline See discussion on scala#4505. The issue will go away when moving to indy-lambda.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Under -Ydelambdafy:method, the name of the anonymous class is
derived from the names of the original enclosing class and
term of the function.
However no effort is made to conform to the
-Xmaxclassnamesettingwhen constructing the class name from the components.
Recently, in 029cce7, I changed uncurry to selectively fallback
to the old method of emitting lambdas when we detect that
-Ydelambdafy:methodwould subvert specialization.As a result of this, we could now have a delambdafy translated
function enclosed in an anonymous class representing an old-style
function. If that function, in turn, is heavily nested, its long
name now contributes a long suffix to the delambdafy class's name.
This led to a "filename too long" error in
run/kmpSliceSearch.scala.I have extract the essence of the failure in
pos/t9319.scala.I then changed delambdafy to use the unexpanded names of the owner
in the suffix, rather than the full name. This makes things work
as expected.
I haven't further protected against pathological cases, given
that in 2.12.x and indylambda, we won't ever emit these classes.