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

cdiv and cmod incorrect? #2643

Open
dubiousjim opened this Issue Oct 4, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@dubiousjim

dubiousjim commented Oct 4, 2018

I was looking at the definitions of cdiv and cmod in Shadow.py. These seem to give incorrect results for some operands. For example, in C99:

4 / -4 == -1    4 % -4 == 0
4 / -2 == -2    4 % -2 == 0
4 / -1 == -4    4 % -1 == 0

But these functions would return:

cdiv(4, -4) == 0    cmod(4, -4) == 4
cdiv(4, -2) == -1    cmod(4, -2) == 2
cdiv(4, -1) == -3    cmod(4, -1) == 1

That's based on just running those definitions in the Python interpreter. Perhaps Cython handles them specially, and doesn't in fact give the incorrect results above. Still, why not include correct Python code? The following would work:

def cdiv(a, b):
    if a < 0:
        a = -a
        b = -b
    if b < 0:
        return (a + b + 1) // b
    return a // b

def cmod(a, b):
    r = a % b
    if (a*b) < 0 and r: r -= b
    return r

Sorry if this is just me misunderstanding some magic behind the scenes stuff.

@robertwb

This comment has been minimized.

Show comment
Hide comment
@robertwb

robertwb Oct 5, 2018

Contributor

Thanks for looking at this! Please feel free to send a PR, or if you don't have the time I can fix them.

Contributor

robertwb commented Oct 5, 2018

Thanks for looking at this! Please feel free to send a PR, or if you don't have the time I can fix them.

@mb-a

This comment has been minimized.

Show comment
Hide comment
@mb-a

mb-a Oct 15, 2018

You might want to make sure that your new functions work properly with the test found here:
https://github.com/cython/cython/blob/master/tests/run/cdivision_CEP_516.pyx#L27-L28

mb-a commented Oct 15, 2018

You might want to make sure that your new functions work properly with the test found here:
https://github.com/cython/cython/blob/master/tests/run/cdivision_CEP_516.pyx#L27-L28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment