-
Notifications
You must be signed in to change notification settings - Fork 3
Initial implementation of FFI/#import for fasm
#52
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: master
Are you sure you want to change the base?
Conversation
farkon00
left a comment
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.
Also you should separate out the changes into multiple commits and the nix changes should be moved to different PR.
| } | ||
|
|
||
| REGULAR_OPTIONS: Dict[str, List[str]] = { | ||
| REGULAR_OPTIONS: Dict[str, Tuple[List[str], bool]] = { |
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.
The type annotation is wrong in master, but the new annotation is wrong as well, it's clearly Dict[str, Tuple[List[str], str]
TODO: make this a proper dataclass, maybe?
| fget=lambda self, name=name: self.config.get( | ||
| name, getattr(self.args, name) | ||
| if getattr(self.args, name) is not None | ||
| else self.REGULAR_OPTIONS[name][1]) |
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.
This should be else self.ARRAY_OPTIONS[name][1]
| if the function is used in code and not by calling the script from command line. | ||
| """ | ||
| config = Config(sys.argv, lsp_mode=lsp_mode) | ||
|
|
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.
Why is there a change here?
| buf = "" | ||
|
|
||
| if op.operand.is_imported: | ||
| calling_convention = ["rdi", "rsi", "rdx", "rcx", "r8", "r9"] |
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.
This should be a constant declared globally
| calling_convention.pop(0) | ||
| in_stack.pop(0) | ||
|
|
||
| buf += f"call {op.operand.name}\npush rax\n" |
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.
You shouldn't push rax if the function does not return a value
| error.name = "ContExitException"; | ||
| return error; | ||
| } | ||
|
|
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.
Why is there a change here?
| else: | ||
| @pytest.mark.parametrize("test_name", tests) | ||
| def test_node_wat64(test_name): | ||
| if test_name == "extern": |
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.
It'd be better if this was a part of the test file itself, but this is a matter for a different PR for sure
| return hash(self.text_repr()) | ||
|
|
||
|
|
||
| class Void(Type): |
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 don't think we need the void type, because we have native untyped pointers in cont
#importsyntax for fasm-lc)