Skip to content
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 converters in sem #399

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jan 21, 2025

closes #381, succeeds #384

Things to do:

  • Implement attaching to structural types or to . when nothing can be attached - moved to compat mode in structural converters in compat mode #401
    • Still attaches to return types as in implement converters in sigmatch #384 but this could be changed to the input type if necessary
    • Can also attach to both the return type and the input type at the same time but this would require 4 key lookups for wildcard ((a, b), (a, .), (., b), (., .))
  • Compare matched converters instead of always considering them ambiguous
  • commonType can instantiate the converter directly instead of calling addArgsInstConverters

Still uses the index as in #384, hopefully not a problem

@metagn
Copy link
Collaborator Author

metagn commented Jan 21, 2025

Undrafting in case the structural type attachment isn't a blocker but will still do it in this PR if it's not merged yet

@metagn metagn marked this pull request as ready for review January 21, 2025 22:52
@Araq
Copy link
Member

Araq commented Jan 21, 2025

Structural type attachment should be behind a --compat switch as it's not part of Nimony's design. (If only we had a spec for it...)

@metagn
Copy link
Collaborator Author

metagn commented Jan 22, 2025

Done in followup as it adds a lot to the diff

@Araq
Copy link
Member

Araq commented Jan 22, 2025

A NIF index file does not need to support the notion of a converter if we embrace a name mangling like <backtick>converter$typecanon. Then the exiting symbol table logic can be used. Alternatively we could treat it as the other =hooks. What do you think?

@metagn
Copy link
Collaborator Author

metagn commented Jan 22, 2025

The main difference with the hooks is that symbols are stored instead of offsets for the corresponding procs which is only because I thought using the offsets first would make the code more complicated (wouldn't be able to make use of prog.mem etc). They could be combined either both using offsets or both using symbols (which already have mappings to offsets anyway I'm guessing)

Using a mangled name would mean that it would be a phantom symbol on top of the existing converter symbol (unless we ignore converter names) which would mean it would have to be manually added to the index anyway and it would also need to work with from import etc

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.

Nimony: add Converters
2 participants