-
Notifications
You must be signed in to change notification settings - Fork 151
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
Implement and use a generics mechanism based on GHC.Generics #284
Conversation
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'm only a short way through reading the changes, but I thought it better to submit my comments so far, than to wait until I've read through the whole thing.
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.
Here's another batch of comments. Still much more to review!
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.
Next batch of comments
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've now reviewed everything except for doDGeneric
and doSGeneric
, which I will get to next. (Did you already commit your changes to share more code between the two? If there are more changes coming, maybe I'll wait until you've pushed them?)
@quark17 I haven't gotten to refactoring |
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.
Here are comments on the latest state of things. (I think I'm all caught up now. But let me know if you were expecting a response or comment on anything.) Thanks!
I note that some of my past comments are still open. Is this because you're still working on it, or because you didn't notice them -- I see that they are hidden behind a couple "Load more" links that have to be clicked to expand, so they could easily be missed.
Thanks - I am still working on some of the older comments. Just getting back to this PR now after chasing down some things in the testsuite. |
…xperiment with vectors
…epSeqCond behavior using primSeqCond
I've finished going through all the comments here, so I think that this and the corresponding testsuite PR are (finally!) ready to merge. |
When I compare these new commits with the previous commits, I see that they differ in one place where you revert a whitespace change (to match the main branch -- great!) but they also differ in the following place:
Was this intended? I recall that we discussed these two options, but I can't find a conversation for it now on this PR. I appreciate that you've done work to reduce the history. Is this all the reducing that you plan to do? I am concerned that it still contains many separate commits that aren't useful. (It still contains at least three or four places where is a choice is reverted by a later commit. And commits that just refactor. And many commits just add comments, or cleanups, or rephrase code, or respond to suggestions, and I would squash all of those into an original commit.) Maybe we should give up on trying to rebase merge. I would be in favor of just doing a squash merge at this point -- unless you think it's worth doing a merge commit, to preserve this history on a branch? But how much is worth recording as separate commits? At best, I'd just have these commits (and even some of these could be merged):
If I did a squash merge and listed the above bullets, would that we OK? |
Or, I could try my hand at using "rebase" to squash all the commits down to that handful or so. |
I vaguely seem to remember someone proposing that change at some point and me doing it. As far as I can tell, this was inadvertently reverted at some point, and ditching the revert-revert commits brought it back? Anyway both options work and the second one seems a bit neater.
I wasn't really sure how granular we wanted to keep this. I tried to take out the obvious white noise (I searched for any commits with "revert" in them) but I may have missed a few. I do sort of want to keep a record of some of my ill-fated experiments and dead ends for historical purposes though, in case I ever write a paper about the challenges involved, or something - not sure if the original commit history is still available anywhere after a rebase/squash merge.
If you want to try feel free to do so - I need to take care of some other stuff and probably won't get back to this before Monday or Tuesday. |
Major changes here:
Generic
type class and associated representation types inPrelude.bs
.Generic
.Generic
is auto-derived on all data, struct and interface types.Vector
andListN
) have custom instances in the standard library that useVector
as their representation type.PrimMakeUndefined
,PrimMakeUninitialized
andPrimDeepSeqCond
in using generics, and remove the existing deriving implementations for them.Generic
instances for primative types, and replacing thePrimDeepSeqCond
instance generated byGenWrap.hs
with a more generalGeneric
instance that uses the original interface type as the representation of the wrapper.Deriving.hs
toPrelude.bs
.CShow
providing a Classic (Haskell)-syntax version ofFShow
.This branch passes the testsuite with only a few minor changes, a corresponding pull request on that repository is coming shortly.
There are a few things that should be discussed before merging this:
WExperimental
warning to uses of theGeneric
in a context as we may decide to tweak the API somewhat in the near future. This is currently being done on thectxRedCQType'
function inCtxRed.hs
. Is this a reasonable place for this check?Generic
type class. However this seems like kind of a strange corner case, is skipping an additionalWExperimental
warning on these reasonable for now?CShow
library live? For now I added it toBase1
since this is a generally-useful feature that has been requested in the past, but I could move it tobsc-contrib
(or somewhere else) if you don't think that it belongs in the standard library. Tests are included in mybsc-testsuite
pull request.CShow
) in the future? Ravi wasn't sure if the special doc comments in the standard library still worked for building the TeX document, and regardless information about how to write them doesn't seem to have been open-sourced.