feat: Add extern symbols #236
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 90.55% 90.68% +0.12%
==========================================
Files 43 44 +1
Lines 4628 4679 +51
==========================================
+ Hits 4191 4243 +52
+ Misses 437 436 -1 ☔ View full report in Codecov by Sentry. |
| module: GuppyModule, | ||
| name: str, | ||
| ty: str, | ||
| symbol: str | None = None, |
There was a problem hiding this comment.
This is a bit dubious, I think name should and symbol should take it's place?
There was a problem hiding this comment.
The motivation behind leaving them distinct was that the LLVM symbol name might not follow Python's naming conventions, so you'd want to pick a different name to refer to it in your program.
But I'm fine with making symbol required and name optional.
There was a problem hiding this comment.
I would be fine with symbol optional, in fact I weakly prefer it(now that I understand better). My reading of the original code was that name did not propagate into the symbol of the ConstExternSymbol. I think you should add tests for how the symbol of ConstExternSymbol is populated with and without the optional param (name or symbol, whichever you decide) here.
|
|
||
| module = GuppyModule("test") | ||
|
|
||
| x = guppy.extern(module, "x", ty="float[int]") |
There was a problem hiding this comment.
I suppose this is impossible?:
@guppy.extern(module, constant = False)
x: float
There was a problem hiding this comment.
Yes, unfortunately decorators are only allowed on functions and classes
tests/integration/test_extern.py
Outdated
| def test_extern_name(validate): | ||
| module = GuppyModule("module") | ||
|
|
||
| ext = guppy.extern(module, "1$weird%symbol", ty="int", name="ext") |
There was a problem hiding this comment.
It's not clear which name is in scope in side the @guppy. Is it the ext python variable name, or the "ext" parameter?
There was a problem hiding this comment.
Only the name parameter matters. The python variable is not considered, you don't even need to assign it
I'm just assigning it to make the linter happy
There was a problem hiding this comment.
I recommend renaming the variable to _ext or something (also the other test and anywhere else), to make this clear.
Closes #169
It's a bit annoying that the type has to be provided as a string, but we can't represent Guppy types as Python objects on user-level yet.