-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Optimise boilerplate generators, use instance constructors #3871
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
09972d8
Refactor KernelBoiler, use SAM instances when possible
joroKr21 5b7bd81
Use instance constructors instead of SAM types
joroKr21 da85e2e
Optimize cats-core boilerplate
joroKr21 8ae7a40
Optimize AlgebraBoilerplate by using instance constructors
joroKr21 5047b08
Merge branch 'main' into sam-boiler
joroKr21 531b51a
Merge branch 'main' into sam-boiler
joroKr21 f24bb2d
Restore AlgebraBoilerplate to classes
joroKr21 1185c8b
Use Show.show
joroKr21 7600153
Simplify GenTupleMonadInstances
joroKr21 6ba7e4b
Convert back multi-method type class instances to anonymous classes
joroKr21 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is there a practical difference between these? e.g. no class is emitted.
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.
Yes, indeed 👍 - because
Show
has only one abstract methodThere 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.
Right, thanks. Is this affected by the aforementioned Scala 2 bug?
Also, can
Semigroup
etc. get the same treatment?cats/kernel/src/main/scala/cats/kernel/Semigroup.scala
Lines 151 to 154 in 6ba7e4b
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 for
Semigroup
it doesn't matter because it has other (non-abstract) methods. So then the bug applies and there would be an anonymous classes generated even if we use the SAM syntax. So the only benefit would be aesthetics.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 see, so it's not just that
Show
has one abstract method, it's that it has no other methods.I agree it's only aesthetics on Scala 2, but does the bug apply to Scala 3 as well?
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 don't remember - that's a good question.
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.
The reason I ask is, I don't care so much about the handful of
Semigroup.instance
etc. that can be re-written.But I'm wondering if in Scala 3 the boilerplate instances themselves could be written directly like this instead of relying on
instance
. So we get the win in terms of jar size without introducing any indirection at all.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.
Even if that's possible it would be quite hard to split like this 😄
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.
FTR Scala 3 doesn't have this bug - but again I'm sceptical about version-specific boilerplate generators 🤔
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.
If there's interest it can be a followup PR. I think it shouldn't be too hard (famous last words): currently all the instances are in the same file I think, when they could easily be split among a few files. And some of those files can go into the scala-2 and scala-3 srcs.