Factor out logic for scala functions. #1962
Conversation
| denot.info = | ||
| ClassInfo(ScalaPackageClass.thisType, cls, ObjectType :: parentTraits, decls) | ||
|
|
||
| decls.enter(newMethod(cls, nme.apply, methodType(tParamTpes.init, tParamTpes.last), Deferred)) |
There was a problem hiding this comment.
With a .init and .last here I'm not actually sure I prefer tParamTpes to argParams & resParam
There was a problem hiding this comment.
Now I agree, the change was done on an intermediate version where it saved a considerable amount of code. This is clearly not the case anymore. I will change it back.
| @@ -688,26 +684,14 @@ class Definitions { | |||
| lazy val AbstractFunctionType = mkArityArray("scala.runtime.AbstractFunction", MaxImplementedFunctionArity, 0) | |||
| val AbstractFunctionClassPerRun = new PerRun[Array[Symbol]](implicit ctx => AbstractFunctionType.map(_.symbol.asClass)) | |||
| def AbstractFunctionClass(n: Int)(implicit ctx: Context) = AbstractFunctionClassPerRun()(ctx)(n) | |||
| private lazy val ImplementedFunctionType = mkArityArray("scala.Function", MaxImplementedFunctionArity, 0) | |||
| lazy val ImplementedFunctionType = mkArityArray("scala.Function", MaxImplementedFunctionArity, 0) | |||
There was a problem hiding this comment.
What about keeping this private and adding:?
lazy val Function0Type = ImplementedFunctionType(0)
lazy val Function1Type = ImplementedFunctionType(1)There was a problem hiding this comment.
This would work nicely. Thought will define them as def as ImplementedFunctionType is an array.
| @@ -780,7 +760,8 @@ class Definitions { | |||
| * trait gets screwed up. Therefore, it is mandatory that FunctionXXL | |||
| * is treated as a NoInit trait. | |||
| */ | |||
| lazy val NoInitClasses = PhantomClasses + FunctionXXLClass | |||
| def isNoInitClass(cls: Symbol) = | |||
| cls.is(NoInitsTrait) || PhantomClasses(cls) || ScalaFunction(cls).exists | |||
There was a problem hiding this comment.
Is this PhantomClasses.contains(cls)?
There was a problem hiding this comment.
I am actually a bit dubious about whether this is an improvement. The refactoring almost doubles the loc dealing with functions but I don't see a clear benefit. It also adds a new concept (in the form of a type) named ScalaFunction. What is it? At the very least this needs to be documented thoroughly.
I would be very reluctant to add new concepts like this, because they increase the cognitive load. So I think this should only be done if we clearly eliminate code duplication by doing it.
|
Yes, in this PR the advantages does not really exist. It is only when adding the functions with phantom parameters that this starts to help. I will redesign it to to be closer to the original code to reduce the cognitive load while making it easy to add the new types of functions. |
nicolasstucki
force-pushed
the
centralize-function-logic
branch
3 times, most recently
from
b2472ea to
09eb2d3
Compare
* Add `isSyntheticFunction` checks for synthetic functions such as FuntionN for N > 22 and ImplicitFunctionN for N >= 0. * Add `erasedFunctionClass` to get the erased verion of synthetic functions. * Change the semantics of `isFunctionClass` to return true if it is any kind of FunctionN or ImplicitFunctionN.
nicolasstucki
force-pushed
the
centralize-function-logic
branch
from
09eb2d3 to
93a2d06
Compare
|
In this PR we:
|
|
LGTM! Great cleanup. 👍 |
This PR factors out code from #1408 aimed to make an abstraction on top of all different kinds of functions. Implemented
Function0toFunction22, syntheticFunctionNwithN > 22,ImplicitFunctionNand erasedFunctionXXL. #1408 extends it withPhantomFunctionMandImplicitPhantomFunctionM.