-
Notifications
You must be signed in to change notification settings - Fork 16
Syntactic Variants #53
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
base: dev-integrity
Are you sure you want to change the base?
Conversation
any insight on the failing test? |
This needs better description of the extent to which this accomplishes #52 (what has been done, what are the currently known limitations, etc). |
I've taken a look, and I'm now wondering why we shouldn't go directly to tuples instead of records? |
I already started this refactoring last weekend. :) |
ok, then let's wait and update the PR once that refactoring is complete. |
Implementation has been refactored to use Tuples |
compiler/src/Basics.hs
Outdated
type VarName = String | ||
type AtomName = String | ||
type DataTypeName = String | ||
type TypeConstructorName = String |
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.
Suggestion for naming: TypeConstructorName
-> VariantConstructorName
compiler/src/Basics.hs
Outdated
type AtomName = String | ||
type DataTypeName = String | ||
type TypeConstructorName = String | ||
type TypeConstructor = (TypeConstructorName, [VarName]) |
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.
Similar to the above. TypeConstructor
-> VariantConstructor
, or even better SyntacticVariantConstructor
to reflect what is going on in here.
compiler/src/Core.hs
Outdated
| LUnit | ||
| LBool Bool | ||
| LAtom AtomName | ||
| LAtom TypeConstructorName |
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 Atoms are to be phased out, then shouldn’t we rename them in the codebase as well?
compiler/src/Direct.hs
Outdated
, RecordPatternMode(..) | ||
, AtomName | ||
, Atoms(..) | ||
, DataTypeName |
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.
let's follow the convention of "syntactic variants"
I've looked through a few files in the codebase. One general comment is that we should avoid using the word "type" if we can; and in this case, one suggested name is "syntactic variants". If that's okay, let's just use that naming consistently throughout the codebase. Some examples are in the code-specific comments inline. Another one "ADTag" -> "SynVariantTag" |
Resolves #52
Tasks
recordstuples.