BUG: ticket #2028: ... #356

Closed
wants to merge 5 commits into
from

Projects

None yet

4 participants

@ericfode

When min_scalar was not checking to see if numbers could fit into unsigned long longs. This was fixed by adding an extra case to function to check if the passed number could fit into a unsigned long long if it could not fit into anything else. Additionaly, regression tests were added.

Cleaned up version of previous pull request

@travisbot

This pull request fails (merged 85104586 into 6a1ab03).

Eric Fode BUG: ticket #2028: When min_scalar was not checking to see if numbers…
… could fit into unsigned long longs. This was fixed by adding an extra case to function to check if the passed number could fit into a unsigned long long if it could not fit into anything else. Additionaly, regression tests were added.
6966bdc
@travisbot

This pull request passes (merged 6966bdc into 6a1ab03).

@charris
Member
charris commented Jul 14, 2012

That didn't seem to work. What did you try?

@ericfode

It works now, i had not rebased it before pushing,

On Sat, Jul 14, 2012 at 2:41 PM, Charles Harris <
reply@reply.github.com

wrote:

That didn't seem to work. What did you try?


Reply to this email directly or view it on GitHub:
#356 (comment)

@teoliphant teoliphant and 1 other commented on an outdated diff Jul 14, 2012
numpy/core/src/multiarray/common.c
return PyArray_DescrFromType(NPY_OBJECT);
- }
- return PyArray_DescrFromType(NPY_LONGLONG);
+ } else {
+ return PyArray_DescrFromType(NPY_LONGLONG);
+ }
}
return NULL;
}
@teoliphant
teoliphant Jul 14, 2012 NumPy member

The indentation on this does not look quite right. Could you check alignment?

Otherwise, this seems like a useful patch.

@charris
charris Jul 14, 2012 NumPy member

They're tabs instead of spaces. Eric, you need to fix your editor.

Also, else like so:

}
else {
@travisbot

This pull request fails (merged 3d31029 into 6a1ab03).

@travisbot

This pull request passes (merged 681bceb into 6a1ab03).

@charris charris and 1 other commented on an outdated diff Jul 15, 2012
numpy/core/src/multiarray/common.c
@@ -36,9 +36,17 @@
/* if integer can fit into a longlong then return that*/
if ((PyLong_AsLongLong(op) == -1) && PyErr_Occurred()) {
PyErr_Clear();
+ if((PyLong_AsUnsignedLongLong(op) == -1) && PyErr_Occurred()){
@charris
charris Jul 15, 2012 NumPy member

There are still hard tabs here, they have to be spaces. If you are using vim, :set list will show the tabs. I assume there is something similar in other editors.

Sorry to be so picky, but once you get a few commits right things will go much easier.

@ericfode
ericfode Jul 15, 2012

No don't worry about being picky it drives me crazy when people don't follow codding standards :).

@travisbot

This pull request passes (merged 120c6bd into 6a1ab03).

@charris charris and 1 other commented on an outdated diff Jul 15, 2012
numpy/core/src/multiarray/common.c
@@ -36,9 +36,17 @@
/* if integer can fit into a longlong then return that*/
if ((PyLong_AsLongLong(op) == -1) && PyErr_Occurred()) {
PyErr_Clear();
+ if((PyLong_AsUnsignedLongLong(op) == -1) && PyErr_Occurred()){
@charris
charris Jul 15, 2012 NumPy member

Getting close ;) These lines need an extra indent. Also, checking an unsigned integer for a negative value should raise a compile warning at minimum. I think the value checks need to be done away with in any case as the documentation doesn't indicate any special return value as a possible error code, but perhaps that is a defect of the documentation.

@charris
charris Jul 15, 2012 NumPy member

OK, I checked the [http://docs.python.org/c-api/long.html](Python 2.7) documentation and -1 indeed indicates a possible error, however the docs also show a cast for the unsigned types, i.e., (unsigned long long) -1.

@ericfode
ericfode Jul 15, 2012

I feel like i am missing what it is that makes these lines need more indents, can you explain?

@charris
charris Jul 15, 2012 NumPy member

The second if is part the body of the first if.

@travisbot

This pull request passes (merged 78f6672 into 6a1ab03).

@charris
Member
charris commented Jul 15, 2012

My bad. I think this should go

else if (PyLong_Check(op)) {
    /* if integer can fit into a longlong then return that*/
    if ((PyLong_AsLongLong(op) == -1) && PyErr_Occurred()) {
        PyErr_Clear();
    }
    else {
        return PyArray_DescrFromType(NPY_LONGLONG);
    }

    if ((PyLong_AsUnsignedLongLong(op) == (unsigned long long) -1) &&
            PyErr_Occurred()) {
        PyErr_Clear();
    }
    else {
         return PyArray_DescrFromType(NPY_ULONGLONG);
    }

    return PyArray_DescrFromType(NPY_OBJECT);
}
@teoliphant teoliphant closed this Jul 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment