Remove array allocation and local array ffi from the Show instance for records #299
Conversation
ajnsit
changed the title
There was a problem hiding this comment.
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?
| @@ -20,6 +23,9 @@ import Type.Proxy (Proxy(..)) | |||
| class Show a where | |||
| show :: a -> String | |||
|
|
|||
| instance showUnit :: Show Unit where | |||
| show _ = "unit" | |||
|
|
|||
There was a problem hiding this comment.
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.
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.
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.
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: