-
Notifications
You must be signed in to change notification settings - Fork 92
Remove array allocation and local array ffi from the Show instance for records #299
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
Remove array allocation and local array ffi from the Show instance for records #299
Conversation
JordanMartinez
left a comment
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.
Correct me if I'm wrong, but can't this code...
show record = case showRecordFields (Proxy :: Proxy ls) record of
"" -> "{}"
r -> "{ " <> r <> " }"be simplified to just
show record = "{" <> showRecordFields (Proxy :: Proxy ls) record) <> "}"if the implementation is updated to the following?
garyb
left a comment
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.
Nice 👍
|
|
||
| instance showUnit :: Show Unit where | ||
| show _ = "unit" | ||
|
|
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.
Could you clarify why this change was also made? And I guess the question is, should it be made?
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.
Same question for Void actually - maybe an earlier iteration of this change needed these in scope for its implementation?
Since all this is in the same library I guess it doesn't matter too much, but the general principle I used years ago when we split the Prelude into modules, was that only instances for Prim types or those that were unavoidably depended upon are introduced in the class module, otherwise they were introduced with the type.
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.
This was needed to avoid cycles in the import tree. I needed the semigroup instance for strings inside the show module, but Data.Semigroup depends on Data.Void and Data.Unit, which in turn depended on Data.Show for defining their show instance.
The rationale for why I resolved the cyclic imports this way is - by removing typeclass dependencies from "core" data structures like Unit and Void, we can trivially ensure they never cause cyclical imports and any typeclasses can always import the data structures. This way we only need to organise and resolve potential cyclical imports between typeclass modules like Data.Show and Data.Semigroup.
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.
Gotcha. Thanks for clarifying!
Description of the change
Removes array allocation from the implementation of show for records, and also consequently, removes the need for local ffi implementations of array functions just to show records.
Checklist: