BUG: Fix some (not all) compiler warnings #9705

eric-wieser · 2017-09-18T07:52:08Z

Many were the downcasting of npy_intp to int, partially because Descr->itemsize has a poor choice of type. The rules seem to be:

Size in bytes - npy_intp (unless it's descr->itemsize...)
Number of operands - int
Number of axes - int

It might be far easier to reason about things if all of these were changed to intp, but perhaps there's performance to be had by not doing that.

Here we throw errors, rather than silently wrapping around.

These were warnings raised by msvc.

Many were the downcasting of `npy_intp` to `int`, partially because `Descr->itemsize` has a poor choice of type. Here we throw errors, rather than silently wrapping around.

eric-wieser · 2017-09-18T07:52:37Z

numpy/core/src/npymath/npy_math_internal.h.src

@@ -659,7 +659,7 @@ npy_divmod@c@(@type@ a, @type@ b, @type@ *modulus)

    /* snap quotient to nearest integral value */
    if (div) {
-        floordiv = npy_floor(div);
+        floordiv = npy_floor@c@(div);


Loss of precision bug here for long double, fixed

Extracted to #9749

eric-wieser · 2017-09-18T07:52:56Z

numpy/core/src/multiarray/ctors.c

+            PyErr_SetString(PyExc_TypeError,
+                    "shape is too long");
+            goto fail;
+        }


Buffer overflow error fixed here

eric-wieser · 2017-09-18T07:53:54Z

numpy/core/src/multiarray/ctors.c

+            PyErr_SetString(PyExc_ValueError, "Buffer too large");
+            goto fail;
+        }
+        descr->elsize = (int) view->itemsize;


This is really frustrating. PyBuffer.itemsize is a ssize_t, but our descr.itemsize is an int. It'd be great if we could fix this our end, but that would be an ABI breakage.

charris · 2017-09-18T16:24:12Z

numpy/core/src/multiarray/_datetime.h

@@ -183,7 +183,7 @@ convert_datetime_metadata_tuple_to_datetime_metadata(PyObject *tuple,
 * the Python datetime.tzinfo object.
 */
 NPY_NO_EXPORT int
-get_tzoffset_from_pytzinfo(PyObject *timezone, npy_datetimestruct *dts);
+get_tzoffset_from_pytzinfo(PyObject *timezone_obj, npy_datetimestruct *dts);


Was reserved word?

Was not consistent with the name in the implementation, and changing this one was easier

charris · 2017-09-18T16:25:22Z

numpy/core/include/numpy/npy_3kcompat.h

@@ -191,7 +191,7 @@ npy_PyFile_Dup2(PyObject *file, char *mode, npy_off_t *orig_pos)
    if (ret == NULL) {
        return NULL;
    }
-    fd2 = PyNumber_AsSsize_t(ret, NULL);
+    fd2 = (int) PyNumber_AsSsize_t(ret, NULL);


I guess we should really have a function for this that could also check that the value is in range. Might make a comment so we can find this later if we want to do a proper fix.

charris · 2017-09-18T16:34:35Z

numpy/core/src/multiarray/arraytypes.c.src

@@ -3693,7 +3693,7 @@ static void

    delta -= start;
    for (i = 2; i < length; ++i) {
-        buffer[i] = start + i*delta;
+        buffer[i] = (@type@) (start + i*delta);


I guess multiplication by i causes the problem here.

charris · 2017-09-18T16:46:09Z

numpy/core/src/multiarray/conversion_utils.c

@@ -945,14 +951,20 @@ PyArray_IntpFromIndexSequence(PyObject *seq, npy_intp *vals, npy_intp maxvals)
 }

 /*NUMPY_API
- * PyArray_IntpFromSequence
+ * PyArray_IntpFromSequence`


Intentional "`"?

Nope, good catch

charris · 2017-09-18T16:47:43Z

numpy/core/src/multiarray/conversion_utils.c

 * Returns the number of integers converted or -1 if an error occurred.
 * vals must be large enough to hold maxvals
 */
 NPY_NO_EXPORT int
 PyArray_IntpFromSequence(PyObject *seq, npy_intp *vals, int maxvals)
 {
-    return PyArray_IntpFromIndexSequence(seq, vals, (npy_intp)maxvals);
+    npy_intp result = PyArray_IntpFromIndexSequence(
+        seq, vals, (npy_intp)maxvals);


Hmm, this might look better broken and aligned.

Are you saying the indentation is wrong?

No, just that it might look better if it started after ( in the usual way. However, it looks like some of these files never got the full style cleanup done many years ago, so probably we can do all that in one swoop at a future date.

charris · 2017-09-18T16:52:20Z

numpy/core/src/multiarray/ctors.c

@@ -666,7 +667,7 @@ discover_dimensions(PyObject *obj, int *maxndim, npy_intp *d, int check_it,
 {
    PyObject *e;
    int r;
-    npy_intp n, i;
+    npy_intp n;


The function signature could use a style fixup.

charris · 2017-09-18T16:56:01Z

numpy/core/src/multiarray/datetime.c

@@ -596,9 +596,9 @@ convert_datetime_to_datetimestruct(PyArray_DatetimeMetaData *meta,
                out->as = (int)((dt % 1000LL) * 1000);
            }
            else {
-                npy_datetime minutes;
+                int minutes;


Needs to be int for the call?

Yep, and we're dividing by such a big number that that seems pretty reasonable

eric-wieser · 2017-09-19T07:16:24Z

numpy/core/src/multiarray/datetime_strings.c

                    int local, int utc, NPY_DATETIMEUNIT base, int tzoffset,
                    NPY_CASTING casting)
 {
    npy_datetimestruct dts_local;
    int timezone_offset = 0;

-    char *substr = outstr, sublen = outlen;


This is the cause of #9712

Extracted to #9814

eric-wieser · 2017-09-19T07:17:49Z

numpy/core/src/multiarray/einsum.c.src

-            _mm_prefetch(data0 + 512, _MM_HINT_T0);
-            _mm_prefetch(data1 + 512, _MM_HINT_T0);
+            _mm_prefetch((char *) (data0 + 512), _MM_HINT_T0);
+            _mm_prefetch((char *) (data1 + 512), _MM_HINT_T0);


This looks very sketchy - was the intended meaning ((char *) data0) + 512? Else we end up loading 4KiB for doubles

I'll guess that what you have is correct as the loop count is in type sized chunks of 8, so it makes some sense that 512 should also be in type sized units. I wonder if it really matters much?

This is very old code and the instruction is Microsoft specific, so I wonder if it isn't worth rewriting the whole thing at some point. @juliantaylor Thoughts?

charris · 2017-09-19T16:30:01Z

Need to use NPY_INTP_FMT instead of d for print formatting after the variable type changes in descriptor.c and datetime_strings.c.

eric-wieser · 2019-03-21T06:43:47Z

Closing this, it stands an approximately 0% chance of being rebaseable

eric-wieser · 2019-03-21T06:50:32Z

numpy/core/src/multiarray/iterators.c

- * out_dimensions, out_strides, and out_offset.
- */
-NPY_NO_EXPORT int
-parse_index(PyArrayObject *self, PyObject *op,


Removed in #12276

eric-wieser · 2019-03-21T07:00:06Z

numpy/core/src/multiarray/mapping.c

+            used_ndim = PyArray_NDIM(self);
+            new_ndim += (int) indices[curr_idx].value;
+            curr_idx += 1;
+        }


Whitespace fixes here moved to #13169

BUG: Fix some (not all) compiler warnings

94dd561

Many were the downcasting of `npy_intp` to `int`, partially because `Descr->itemsize` has a poor choice of type. Here we throw errors, rather than silently wrapping around.

eric-wieser commented Sep 18, 2017

View reviewed changes

charris added 00 - Bug component: numpy._core labels Sep 18, 2017

charris reviewed Sep 18, 2017

View reviewed changes

eric-wieser mentioned this pull request Sep 19, 2017

Casting datetime to string truncates modulo 256 #9712

Closed

More fixes

d3fa816

eric-wieser commented Sep 19, 2017

View reviewed changes

eric-wieser mentioned this pull request Oct 19, 2017

BUG: add.reduce gives wrong results for arrays with funny strides #9887

Merged

eric-wieser added 52 - Inactive Pending author response 55 - Needs work labels Nov 14, 2017

eric-wieser mentioned this pull request Dec 16, 2017

Tons of new nitpicky warning failures in daily build test. #10222

Closed

eric-wieser closed this Mar 21, 2019

eric-wieser commented Mar 21, 2019

View reviewed changes

eric-wieser mentioned this pull request Mar 21, 2019

STY: Fix weird indents to be multiples of 4 spaces #13169

Merged

eric-wieser commented Mar 21, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix some (not all) compiler warnings #9705

BUG: Fix some (not all) compiler warnings #9705

eric-wieser commented Sep 18, 2017 •

edited

Loading

eric-wieser Sep 18, 2017 •

edited

Loading

eric-wieser Sep 23, 2017

eric-wieser Sep 18, 2017

eric-wieser Sep 18, 2017

charris Sep 18, 2017

eric-wieser Sep 18, 2017

charris Sep 18, 2017

charris Sep 18, 2017

charris Sep 18, 2017 •

edited

Loading

eric-wieser Sep 18, 2017

charris Sep 18, 2017 •

edited

Loading

eric-wieser Sep 18, 2017

charris Sep 18, 2017

charris Sep 18, 2017

charris Sep 18, 2017

eric-wieser Sep 18, 2017

eric-wieser Sep 19, 2017

eric-wieser Oct 3, 2017

eric-wieser Sep 19, 2017 •

edited

Loading

charris Sep 19, 2017

charris Sep 19, 2017

charris commented Sep 19, 2017

eric-wieser commented Mar 21, 2019

eric-wieser Mar 21, 2019

eric-wieser Mar 21, 2019

BUG: Fix some (not all) compiler warnings #9705

BUG: Fix some (not all) compiler warnings #9705

Conversation

eric-wieser commented Sep 18, 2017 • edited Loading

eric-wieser Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Sep 19, 2017

eric-wieser commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Sep 18, 2017 •

edited

Loading

eric-wieser Sep 18, 2017 •

edited

Loading

charris Sep 18, 2017 •

edited

Loading

charris Sep 18, 2017 •

edited

Loading

eric-wieser Sep 19, 2017 •

edited

Loading