-
Notifications
You must be signed in to change notification settings - Fork 61
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
Use imp summon macro to summon type class instance for apply #49
Conversation
👍 Awesome! Looks good to me. @non @milessabin Any comments before merging? |
I think it'll make sense to share implementation in Typeclassic eventually, but in this case I think you'd do better just to make your |
Cool, I can definitely look at doing something like that if you folks think it is better. With that said, what's the rationale that means it would be better just to do it directly? The imp macro has tests to verify the bytecode generated is as expected, and it would be a shame to have to duplicate those tests here - or even worse just assume that the same result is achieved, when leveraging the imp library means that the expectation is already verified. Granted it does not seem like a major duplication but still interested in the rationale, e.g. is it something to do with adding an extra compile time dependency on users? |
I'd like to hear more rationale too. I'm typically very dependency adverse, but in this case, given that simulacrum and imp will eventually be combined in typeclassic, I don't see any major downsides. |
Well, it's really that the local implementation will be less lines of code/sbt than the dependency :-) |
It's really just, def apply[$tparam](implicit ev: ${typeClass.name}[${tparam.name}]): ${typeClass.name}[${tparam.name}] =
macro MacroObj.applyImpl
object MacroObject { def applyImpl[T](implicit c: Context)(t: c.Expr[T]): c.Expr[T] = t } |
For sure, that seems a small amount of code, and looks like it would directly emulate current imp behaviour. FWIW, I personally think I prefer the dependency. Amongst other things, if non/imp#2 (comment) is solved to provide a better variant for scala 2.11, and if that happens before typeclassic (not sure when that is happening??) then an imp upgrade should provide the optimized behavior in a relatively straightforward fashion. BTW - I did spend a few minutes looking at porting the code above into simulacrum. The object that defines the macro needs to be a stable object I think, i.e. the |
OK let's go with this PR then. @mikejcurry one minor issue -- we need to remove |
Cool, that should be done now. Good catch on the |
Use imp summon macro to summon type class instance for apply
This PR is to address #48
Happy to tweak it if there is a better way to do this with macros - have never written scala macro code before.
Figured the best way to learn is to write some…