-
Notifications
You must be signed in to change notification settings - Fork 27
Conform floor division and modulo to Python semantics #251
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
Conversation
|
||
def make_ops(T: str, pyclass: type[W_Object]) -> None: | ||
w_T = pyclass._w # e.g. B.w_i32 | ||
WT = Annotated[W_IntLike, w_T] |
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.
do you have any specific reason for killing this?
I personally find having Annotated[W_IntLike, w_T]
everywhere hard to read, and this patterns is used a bit everywhere, e.g. here:
Lines 61 to 78 in ead6d95
@builtin_method('__get__', color='blue', kind='metafunc') | |
@staticmethod | |
def w_GET( | |
vm: 'SPyVM', wam_self: 'W_MetaArg', wam_obj: 'W_MetaArg' | |
) -> 'W_OpSpec': | |
from spy.vm.opspec import W_OpSpec | |
w_self = wam_self.w_blueval | |
assert isinstance(w_self, W_Member) | |
w_T = wam_obj.w_static_T | |
field = w_self.field # the interp-level name of the attr (e.g, 'w_x') | |
T = Annotated[W_Object, w_T] # type of the object | |
V = Annotated[W_Object, w_self.w_type] # type of the attribute | |
@vm.register_builtin_func(w_T.fqn, f"__get_{w_self.name}__") | |
def w_get(vm: 'SPyVM', w_obj: T) -> V: | |
return getattr(w_obj, field) | |
return W_OpSpec(w_get, [wam_obj]) |
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.
@antocuni I was running up against Pylance Type error Variable not allowed in type expression
which made it hard to work with. I've just changed the language server to Jedi and it works ok with that pattern so I'm happy to revert it.
It could be good to have the recommended static type checker (mypy) and language server on the readme for local development because they're all slightly different and the default language server on vscode is Pylance.
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 recommend mypy: this is what the CI tests and it's the only one "officially" supported.
I'm not opposed to switch to another type checker, but to do so we would need to understand pros&cons and to test it in CI.
Ironically, the fact that in Python "type checking" is not a well defined concept and that every typechecker implements slightly different rules is exactly one of the things that SPy aims to solve :).
But for that, we need to wait until SPy is powerful enough to compile itself :)
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.
LGTM, apart the assert
s
This PR correctly implements floor division (//) and modulo (%) operators, fully matching Python’s semantics for both integers and floats.
Changes include:
Now it uses pure integer arithmetic which matches CPython's internal implementation and ensures negative values round correctly toward negative infinity.
Guarantees that the result of
x % y
always has the same sign asy
consistent with Python.This closes #238