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

Bug: operations on ImmutableCintX should not return immutable types #19

Open
disconnect3d opened this issue Nov 25, 2019 · 2 comments
Open
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@disconnect3d
Copy link
Owner

TLDR:

In [10]: a = cint.I32(-1) * cint.I32.MIN

In [11]: a += 1
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-11-8076006b0952> in <module>
----> 1 a += 1

/usr/local/lib/python3.7/site-packages/cint/cint.py in __not_implemented__(self, _)
    193     class ImmutableCintX(type_, ImmutableCint):
    194         def __not_implemented__(self, _):
--> 195             raise NotImplementedError
    196
    197         __iadd__ = __not_implemented__

NotImplementedError:

In [12]: a
Out[12]: ImmutableCintX(-2147483648)

The expected behavior is that a should be an I32 here.

Note that the presented behavior works fine if we multiple immutable value with a "bigger" or "stronger" type. E.g.:

In [13]: a = cint.I64(-1) * cint.I32.MIN

In [14]: a += 1

In [15]:

This can be solved in two ways:

  1. Mutable types should be considered as "stronger" in https://github.com/disconnect3d/cint/blob/master/cint/cint.py#L59-L66

  2. Immutable types should implement __i...__ magic methods and return mutable types; though this might be counter-intuitive?

@disconnect3d disconnect3d added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Nov 25, 2019
@gbdlin
Copy link
Contributor

gbdlin commented Nov 25, 2019

Operations on immutable types should never return mutable types, unless specified somehow explicitly.

And yes, immutable types should implement __i...__ methods. Those methods should return new, immutable number.

If your goal is to implement C integers that are trivial to use (like default python integer), they should behave as python integer whenever it is possible.

@disconnect3d
Copy link
Owner Author

If your goal is to implement C integers that are trivial to use (like default python integer), they should behave as python integer whenever it is possible.

The goal is to have C integer semantics in Python.

I forgot about it when writing this isssue but currently there is no (direct) mutability (unless you use obj.value += 2) as the __i...__ methods return new objects anyway. As in:
image

This being said we should probably rename ImmutableCint/ImmutableCintX to e.g. I32Const, I64Const etc and not overwrite the __i...__ methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants