Skip to content

Conversation

@jaredponn
Copy link
Contributor

@jaredponn jaredponn commented Dec 1, 2023

This PR changes

newtype PrintRead qvn = MkPrintRead { builtins :: Map ValueName qvn }

to

newtype PrintRead qvn = MkPrintRead { builtins :: Ref -> Maybe qvn }

s.t. the generated qvn may be influenced by the types it is instantiated with.

This is important because in languages without type classes, we need to explicitly say which instance should be used. For example if we have type

prod Dog = Dog Int Char

when we generate the Eq instance, writing something like

eq (Dog a0 b0)  (Dog a1 b1) = a0 `eq` a1 && b0 `eq` b1

won't suffice, as we must provide the type information of Int and Char to a0 `eq` a1 and b0 `eq` b1 respectively. We would need to write something like

eq (Dog a0 b0)  (Dog a1 b1) = a0 `eqInt` a1 && b0 `eqChar` b1

Admittedly, I see this as a temporary fix since the builtins really should provide the types they may be instantiated with e.g. we shouldn't just have eq as a builtin -- we should have eq instantiated with typesInt, Char, blah blah blah, are all builtins

@jaredponn jaredponn force-pushed the jared/generalize-builtin-symbol-table-lookup branch from 0733730 to 57171ea Compare December 1, 2023 00:33
changing `newtype PrintRead qvn = MkPrintRead { builtins :: Map ValueName qvn }`
to `newtype PrintRead qvn = MkPrintRead { builtins :: Ref -> Maybe qvn }`
s.t. the generated `qvn` may be influenced by the types it is
instantiated with.
@jaredponn jaredponn force-pushed the jared/generalize-builtin-symbol-table-lookup branch from 57171ea to ae84abc Compare December 1, 2023 00:39
Copy link
Contributor

@bladyjoker bladyjoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Waiting for @szg251

@bladyjoker
Copy link
Contributor

I think this is fairly uncontroversial change and @szg251's work in Rust codegen will not suffer for it. I'll go ahead an merge this.

@bladyjoker bladyjoker added this pull request to the merge queue Dec 1, 2023
Merged via the queue into main with commit 506fe69 Dec 1, 2023
@bladyjoker bladyjoker deleted the jared/generalize-builtin-symbol-table-lookup branch December 1, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants