Split subtypes array initialization to multiple functions. #485
Conversation
| (t: $genericType) => t.isInstanceOf[$subType], | ||
| (t: $genericType) => t.asInstanceOf[$subType] |
There was a problem hiding this comment.
You can extract those as helper methods in the beginning to reduce the method size
There was a problem hiding this comment.
I guess it's not so easy, because if we make helper methods they need to deal with ClassTag 🤔
There was a problem hiding this comment.
Ah, I understand what you mean. Yes, it seems we need to generate sort of runtime reflection codes by macro if we want extract that codes as helper method. So I would say keeping the current way is better even if the runtime reflection code would reduce the method size dramatically. What do you think?
There was a problem hiding this comment.
Yes, let's keep it like this
|
@joroKr21 I fixed based on what you commented. Could you review this PR again? |
| val assignments = typeclasses.zipWithIndex.map { case ((subType, typeclass), idx) => | ||
| val symbol = subType.typeSymbol | ||
| val (annotationTrees, inheritedAnnotationTrees) = annotationsOf(symbol) | ||
| q"""arr(start + $idx) = $SubtypeObj[$typeConstructor, $genericType, $subType]( |
There was a problem hiding this comment.
arr and start need to be fresh names for hygiene.
There was a problem hiding this comment.
be8bd49
I fixed it. Could you explain me the reason why we need fresh names even if they are local variables of that method?
There was a problem hiding this comment.
Because the user code might contain the same names. It's just insurance.
| $ArrayObj(..${annotationTrees}): Array[_root_.scala.Any], | ||
| $ArrayObj(..${inheritedAnnotationTrees}): Array[_root_.scala.Any], | ||
| $ArrayObj(..${typeAnnotationsOf(symbol, fromParents = true)}): Array[_root_.scala.Any], |
There was a problem hiding this comment.
Again for hygiene:
| $ArrayObj(..${annotationTrees}): Array[_root_.scala.Any], | |
| $ArrayObj(..${inheritedAnnotationTrees}): Array[_root_.scala.Any], | |
| $ArrayObj(..${typeAnnotationsOf(symbol, fromParents = true)}): Array[_root_.scala.Any], | |
| $ArrayObj[$AnyTpe](..${annotationTrees}), | |
| $ArrayObj[$AnyTpe](..${inheritedAnnotationTrees}), | |
| $ArrayObj[$AnyTpe](..${typeAnnotationsOf(symbol, fromParents = true)}), |
| )""" | ||
| } | ||
| val tree = | ||
| q"""def $name(arr: Array[$arrayType], start: Int) = { |
There was a problem hiding this comment.
Extract this to the top level:
val ArrayTpe = typeOf[Array[Any]].typeConstructorAnd use it like this:
| q"""def $name(arr: Array[$arrayType], start: Int) = { | |
| q"""def $name(arr: $ArrayTpe[$arrayType], start: $IntTpe) = { |
|
Thanks! Release is in progress |
We are using Magnolia in production and mainly use it to derive the parent type's type class instances from the type class instances of subtypes. One of our types has more than 700 subtypes, and when derives typeclass instances of this type, the process of initializing the huge subtypes array is performed in a single method, resulting in a "Method too large" compilation error. This PR splits the initialization of the subtypes array into multiple functions and calls each function at the end to combine the array. This method also increases the size of the method, but to a much slower degree, so I think it's still worthwhile. I would appreciate it if you could review it.