Implements Type to BType conversion. #1145
Conversation
|
Test FAILed. |
| case sym if sym == definitions.NothingClass => "Lscala/Nothing;" | ||
| case sym if sym.isTypeParameterOrSkolem => "Ljava/lang/Object;" | ||
| case definitions.ArrayClass => ARRAY_TAG + toJavaDescriptor(args.head) | ||
| case _ => OBJECT_TAG + sym.javaClassName.replace('.', '/')+";" |
There was a problem hiding this comment.
is javaClassName.replace... better than javaBinaryName?
There was a problem hiding this comment.
javaBinaryNameString seems to be optimal
There was a problem hiding this comment.
actually looks like they are giving same string. I look into it deeper. Anyway test with List is failing so there is space to make thing better.
Other thing is Lscala/Null;, I saw in code something like Lscala/runtime/Null;. I have to verify it
wpopielarski
force-pushed
the
2.12-compatibility
branch
from
fbb5978
to
0e26784
Compare
| if (prefix != "") prefix + "." + tpe.sym.nameString | ||
| import rootMirror.getPackageIfDefined | ||
| val pkg = getPackageIfDefined(TermName(prefix)) | ||
| if (prefix != "" && pkg != definitions.ScalaPackage) prefix + "." + tpe.sym.nameString |
There was a problem hiding this comment.
What is this doing exactly? Can you give an example? Is it omitting the scala prefix?
There was a problem hiding this comment.
yes, it is skipping scala package
There was a problem hiding this comment.
Aha! Right where my investigation ended :)
There was a problem hiding this comment.
I'm not sure if I like like this change. When is it useful to not display the scala package?
There was a problem hiding this comment.
There are test failures because without this change scala.Predef.String is displayed as scala.String as opposed to just String
wpopielarski
force-pushed
the
2.12-compatibility
branch
from
0e26784
to
4f6ef77
Compare
|
Test FAILed. |
|
There are lots of failing tests on Jenkins. What a nuisance. |
|
Test FAILed. |
| case DOUBLE => C_DOUBLE.toString | ||
| case ClassBType(cls) => s"L$cls;" | ||
| case ArrayBType(elem) => s"[${javaDescriptor(elem)}" | ||
| private def toJavaDescriptor(tpe: Type): String = { |
There was a problem hiding this comment.
@lrytz Could you have a look at this function? Maybe you can tell us if it is a good replacement for the function above. We want to get rid of BType but it has quite a complex implementation and therefore we do not know if we covered all cases.
There was a problem hiding this comment.
What is the compiler phase this function is running at?
| case DOUBLE => C_DOUBLE.toString | ||
| case ClassBType(cls) => s"L$cls;" | ||
| case ArrayBType(elem) => s"[${javaDescriptor(elem)}" | ||
| private def toJavaDescriptor(tpe: Type): String = { |
There was a problem hiding this comment.
What is the compiler phase this function is running at?
| import scala.reflect.internal.ClassfileConstants._ | ||
| val (sym, args) = tpe match { | ||
| case TypeRef(_, sym, args) => (sym, args) | ||
| case rt @ RefinedType(_, _) => (rt.typeSymbol, rt.typeArgs) |
There was a problem hiding this comment.
This match is very non-exhaustive - how can you know that no other types reach here?
There was a problem hiding this comment.
The function runs in the presentation compiler. We don't have access to the backend here.
There was a problem hiding this comment.
What I mean is types like ThisType, ConstantType, ExistentialType, AnnotatedType, etc: https://github.com/scala/scala/blob/v2.12.1/src/reflect/scala/reflect/internal/Types.scala#L22-L75
…o make code Scala Version agnostic.
|
Test FAILed. |
|
Depends on scala/scala#5773 |
|
retest this please |
kiritsuku
changed the title
|
Tests are still failing on Jenkins. I merge anyway, we can look at this in a different PR. |
There is still failing test for
ScalaJavaMapperDepends on scala/scala#5773