-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Avoid allocations for Monoid
instances where possible
#4309
Conversation
|
||
object ListMonoid { | ||
@nowarn("msg=deprecated") | ||
private val singleton: Monoid[List[Any]] = new ListMonoid[Any] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private val singleton: Monoid[List[Any]] = new ListMonoid[Any] | |
private[this] val singleton: Monoid[List[Any]] = new ListMonoid[Any] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought private[this]
was in line for deprecation in Scala 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private[this]
and private
are effectively the same in Scala 3. But it still means in something in Scala 2.
private val singleton: Monoid[Chain[Any]] = new Monoid[Chain[Any]] { | ||
def empty: Chain[Any] = Chain.nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need an object for this? can we just put it alongside the implicit def
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not - I did it for consistency with the other cases but happy to change it.
That also raises the question of whether to go ahead with adding the companion objects in the other cases. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. In those cases the monoid class was publicly exposed. In this case the object is private, seems to be created unnecessary indirection
Tbh I just don't think it's that valuable—if users directly want the monoid instance can't they just grab it from the implicit def instances
or whatever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you run into bincompat issues if you put it on the trait. Or at least I did in #4313
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, but should be fine if either the trait is private/sealed or it's actually an (abstract) class 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha 👍 Whereas ListInstances
is a public trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for those we will have to store the instance elsewhere 👍
OK I've made the changes above. How do I actually add the commits to this PR? 😅 Do I need push access to Ben's fork or can I push to some magical branch on the typelevel repo? |
@TimWSpence you can just open a new PR based on this branch. |
Prompted by http4s/http4s#6712, which locally caches a particular
Monoid[List[*]]
instance - the issue is a generic one so it seems better to apply this optimisation here.There are other cases where this kind of change could be made too but I wanted to get feedback before doing any more work on it.