ronantakizawa/codereview-bench huggingface dataset:
Repository: numpy/numpy
File: numpy/_core/src/umath/ufunc_object.c (C)

Code before:
        return NULL;
    }

    signature = PyObject_CallFunctionObjArgs(
            npy_runtime_imports._ufunc_inspect_signature_builder,
            (PyObject *)ufunc, NULL);
    if (signature == NULL) {
        return NULL;
    }

    // Cache the result in the instance dict for next time
    if (PyDict_SetItem(ufunc->dict, npy_interned_str.__signature__,
                       signature) < 0) {
        Py_DECREF(signature);
        return NULL;
    }

    return signature;
}

static PyObject *
ufunc_getattro(PyObject *obj, PyObject *name)
{
    // __signature__ special-casing to prevent class attribute access
    if (PyUnicode_Check(name) &&
        PyUnicode_CompareWithASCIIString(name, "__signature__") == 0) {
        return ufunc_get_inspect_signature((PyUFuncObject *)obj, NULL);
    }
    
    // For all other attributes, use default behavior
    return PyObject_GenericGetAttr(obj, name);
}

static PyObject *
ufunc_get_nin(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored))
{
    return PyLong_FromLong(ufunc->nin);
}

static PyObject *
ufunc_get_nout(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored))
{
    return PyLong_FromLong(ufunc->nout);
}

static PyObject *
ufunc_get_nargs(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored))
{
    return PyLong_FromLong(ufunc->nargs);
}


Review comment (performance):
Maybe compare with `npy_interned_str.__signature__`?

Aside: interesting that it needs the attribute getter, I had not thought of this... is it needed just for the caching in `__dict__`? since signature access is unlikely to be frequent or performance critical, I think it is actually fine not to cache. But this is OK too!

Code after:
        return NULL;
    }

    signature = PyObject_CallFunctionObjArgs(
            npy_runtime_imports._ufunc_inspect_signature_builder,
            (PyObject *)ufunc, NULL);
    if (signature == NULL) {
        return NULL;
    }

    // Cache the result in the instance dict for next time
    if (PyDict_SetItem(ufunc->dict, npy_interned_str.__signature__,
                       signature) < 0) {
        Py_DECREF(signature);
        return NULL;
    }

    return signature;
}

static PyObject *
ufunc_getattro(PyObject *obj, PyObject *name)
{
    // __signature__ special-casing to prevent class attribute access
    if (PyUnicode_Check(name) &&
        PyUnicode_Compare(name, npy_interned_str.__signature__) == 0) {
        return ufunc_get_inspect_signature((PyUFuncObject *)obj, NULL);
    }
    
    // For all other attributes, use default behavior
    return PyObject_GenericGetAttr(obj, name);
}

static PyObject *
ufunc_get_nin(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored))
{
    return PyLong_FromLong(ufunc->nin);
}

static PyObject *
ufunc_get_nout(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored))
{
    return PyLong_FromLong(ufunc->nout);
}

static PyObject *
ufunc_get_nargs(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored))
{
    return PyLong_FromLong(ufunc->nargs);
}
Repository: numpy/numpy
File: numpy/_core/src/common/npy_hashtable.cpp (C++)

Code before:
        }
        tb_item[0] = value;
        memcpy(tb_item+1, key, tb->key_len * sizeof(PyObject *));
        tb->nelem += 1;
    }
    else {
        /* Clear the bucket -- just the value should be enough though. */
        memset(tb_item, 0, (tb->key_len + 1) * sizeof(PyObject *));
    }

    return 0;
}


NPY_NO_EXPORT PyObject *
PyArrayIdentityHash_GetItem(PyArrayIdentityHash *tb, PyObject *const *key)
{
    PyObject *res = find_item(tb, key)[0];
    return res;
}

NPY_NO_EXPORT PyObject *
PyArrayIdentityHash_GetItemWithLock(PyArrayIdentityHash *tb, PyObject *const *key)
{
    PyObject *res;
#ifdef Py_GIL_DISABLED
    std::shared_mutex *mutex = (std::shared_mutex *)tb->mutex;
    NPY_BEGIN_ALLOW_THREADS
    mutex->lock_shared();
    NPY_END_ALLOW_THREADS
#endif
    res = find_item(tb, key)[0];
#ifdef Py_GIL_DISABLED
    mutex->unlock_shared();
#endif
    return res;
}


Review comment (refactor):
Can you move this ifdef over to the call site in `ufunc_object.c` and make the `else` case call `PyArrayIdentityHash_GetItem`? That makes it a little clearer there's only locking happening on the free-threaded build. It also follows the pattern in dispatching.cpp where we only call `promote_and_get_info_and_ufuncimpl_with_locking` on the free-threaded build.

I also think the logic in `PyArrayIdentityHash_GetItemWithLock` is shared with the first half of the implementation of `promote_and_get_info_and_ufuncimpl_with_locking`, so you can call `PyArrayIdentityHash_GetItemWithLock` and delete some duplicated code over in that function.

Code after:
        }
        tb_item[0] = value;
        memcpy(tb_item+1, key, tb->key_len * sizeof(PyObject *));
        tb->nelem += 1;
    }
    else {
        /* Clear the bucket -- just the value should be enough though. */
        memset(tb_item, 0, (tb->key_len + 1) * sizeof(PyObject *));
    }

    return 0;
}


NPY_NO_EXPORT PyObject *
PyArrayIdentityHash_GetItem(PyArrayIdentityHash *tb, PyObject *const *key)
{
    PyObject *res = find_item(tb, key)[0];
    return res;
}

#ifdef Py_GIL_DISABLED

NPY_NO_EXPORT PyObject *
PyArrayIdentityHash_GetItemWithLock(PyArrayIdentityHash *tb, PyObject *const *key)
{
    PyObject *res;
    std::shared_mutex *mutex = (std::shared_mutex *)tb->mutex;
    NPY_BEGIN_ALLOW_THREADS
    mutex->lock_shared();
    NPY_END_ALLOW_THREADS
    res = find_item(tb, key)[0];
    mutex->unlock_shared();
    return res;
}

#endif // Py_GIL_DISABLED
Repository: numpy/numpy
File: numpy/_core/numeric.py (Python)

Code before:
    before "sliding" the two across one another:

    >>> import numpy as np
    >>> np.convolve([1, 2, 3], [0, 1, 0.5])
    array([0. , 1. , 2.5, 4. , 1.5])

    Only return the middle values of the convolution.
    Contains boundary effects, where zeros are taken
    into account:

    >>> np.convolve([1,2,3],[0,1,0.5], 'same')
    array([1. ,  2.5,  4. ])

    The two arrays are of the same length, so there
    is only one position where they completely overlap:

    >>> np.convolve([1,2,3],[0,1,0.5], 'valid')
    array([2.5])

    """
    a, v = array(a, copy=None, ndmin=1), array(v, copy=None, ndmin=1)
    if len(a) == 0:
        raise ValueError('a cannot be empty')
    if len(v) == 0:
        raise ValueError('v cannot be empty')
    if (len(v) > len(a)):
        a, v = v, a
    return multiarray.correlate(a, v[::-1], mode)


def _outer_dispatcher(a, b, out=None):
    return (a, b, out)


@array_function_dispatch(_outer_dispatcher)
def outer(a, b, out=None):
    """
    Compute the outer product of two vectors.

    Given two vectors `a` and `b` of length ``M`` and ``N``, respectively,
    the outer product [1]_ is::

      [[a_0*b_0  a_0*b_1 ... a_0*b_{N-1} ]
       [a_1*b_0    .
       [ ...          .
       [a_{M-1}*b_0            a_{M-1}*b_{N-1} ]]

    Parameters
    ----------
    a : (M,) array_like
        First input vector.  Input is flattened if

Review comment (suggestion):
```suggestion
    if len(v) > len(a):
```

might as well :shrug: 

Code after:
    before "sliding" the two across one another:

    >>> import numpy as np
    >>> np.convolve([1, 2, 3], [0, 1, 0.5])
    array([0. , 1. , 2.5, 4. , 1.5])

    Only return the middle values of the convolution.
    Contains boundary effects, where zeros are taken
    into account:

    >>> np.convolve([1,2,3],[0,1,0.5], 'same')
    array([1. ,  2.5,  4. ])

    The two arrays are of the same length, so there
    is only one position where they completely overlap:

    >>> np.convolve([1,2,3],[0,1,0.5], 'valid')
    array([2.5])

    """
    a, v = array(a, copy=None, ndmin=1), array(v, copy=None, ndmin=1)
    if len(a) == 0:
        raise ValueError('a cannot be empty')
    if len(v) == 0:
        raise ValueError('v cannot be empty')
    if len(v) > len(a):
        a, v = v, a
    return multiarray.correlate(a, v[::-1], mode)


def _outer_dispatcher(a, b, out=None):
    return (a, b, out)


@array_function_dispatch(_outer_dispatcher)
def outer(a, b, out=None):
    """
    Compute the outer product of two vectors.

    Given two vectors `a` and `b` of length ``M`` and ``N``, respectively,
    the outer product [1]_ is::

      [[a_0*b_0  a_0*b_1 ... a_0*b_{N-1} ]
       [a_1*b_0    .
       [ ...          .
       [a_{M-1}*b_0            a_{M-1}*b_{N-1} ]]

    Parameters
    ----------
    a : (M,) array_like
        First input vector.  Input is flattened if
Repository: numpy/numpy
File: numpy/_core/src/common/raii_utils.hpp (C++)

Code before:
//
// instead of
//
//     PyGILState_STATE gil_state =  PyGILState_Ensure();
//     [code that uses the Python C API here]
//     PyGILState_Release(gil_state);
//
// or
//     NPY_ALLOW_C_API_DEF
//     NPY_ALLOW_C_API
//     [code that uses the Python C API here]
//     NPY_DISABLE_C_API
//
// This ensures that PyGILState_Release(gil_state) is called,  even if the
// wrapped code throws an exception or executes a return or a goto.
//
class EnsureGIL
{
    PyGILState_STATE gil_state;

public:

    EnsureGIL() {
        gil_state = PyGILState_Ensure();
    }

    ~EnsureGIL() {
        PyGILState_Release(gil_state);
    }
};


//
// RAII for Python thread state.
//
// In C++ code, use this at the beginning of a scope, e.g.
//
//     {
//         np::raii::SaveThreadState save_thread_state{};
//         [code...]
//     }
//
// instead of
//
//     PyThreadState *thread_state = PyEval_SaveThread();
//     [code...]
//     PyEval_RestoreThread(thread_state);
//
// or
//     Py_BEGIN_ALLOW_THREADS
//     [code...]

Review comment (suggestion):
You probably want `EnsureGIL(EnsureGIL const&) = delete;` too (and the assignment version, and perhaps also the `&&` version), to avoid doubly-releasing.

Code after:
//
// instead of
//
//     PyGILState_STATE gil_state =  PyGILState_Ensure();
//     [code that uses the Python C API here]
//     PyGILState_Release(gil_state);
//
// or
//     NPY_ALLOW_C_API_DEF
//     NPY_ALLOW_C_API
//     [code that uses the Python C API here]
//     NPY_DISABLE_C_API
//
// This ensures that PyGILState_Release(gil_state) is called,  even if the
// wrapped code throws an exception or executes a return or a goto.
//
class EnsureGIL
{
    PyGILState_STATE gil_state;

public:

    EnsureGIL() {
        gil_state = PyGILState_Ensure();
    }

    ~EnsureGIL() {
        PyGILState_Release(gil_state);
    }

    EnsureGIL(const EnsureGIL&) = delete;
    EnsureGIL(EnsureGIL&& other) = delete;
    EnsureGIL& operator=(const EnsureGIL&) = delete;
    EnsureGIL& operator=(EnsureGIL&&) = delete;
};


//
// RAII for Python thread state.
//
// In C++ code, use this at the beginning of a scope, e.g.
//
//     {
//         np::raii::SaveThreadState save_thread_state{};
//         [code...]
//     }
//
// instead of
//
//     PyThreadState *thread_state = PyEval_SaveThread();
//     [code...]
Repository: numpy/numpy
File: numpy/_core/src/common/raii_utils.hpp (C++)

Code before:
NpyString_release_allocator(npy_string_allocator *allocator);


namespace np { namespace raii {

//
// RAII for holding a Python reference.
// This is a wrapper for Py_INCREF/Py_DECREF.
//
// In C++ code, use this at the beginning of a scope, e.g.
//
//     {
//         np::raii::ScopedINCREF(obj);
//         [code that requires holding a reference to obj]
//     }
//
// instead of
//
//     Py_INCREF(obj);
//     [code that requires holding a reference to obj]
//     Py_DECREF(obj);
//
// This ensures that Py_DECREF(obj) is called, even if the wrapped
// code throws an exception or executes a return or a goto.
//
class ScopedINCREF
{
    PyObject* _obj;

public:

    ScopedINCREF(PyObject *obj) : _obj(obj) {
        Py_INCREF(_obj);
    }

    ~ScopedINCREF() {
        Py_DECREF(_obj);
    }

    ScopedINCREF(const ScopedINCREF&) = delete;
    ScopedINCREF(ScopedINCREF&& other) = delete;
    ScopedINCREF& operator=(const ScopedINCREF&) = delete;
    ScopedINCREF& operator=(ScopedINCREF&&) = delete;
};

//
// RAII for PyGILState_* API.
//
// In C++ code, use this at the beginning of a scope, e.g.
//
//     {

Review comment (suggestion):
For this one I would prefer the idea of an `OwnedRef` or so object that allows a `.get()` and a `.release()` (one giving the `PyObject *`, the other returning it but invalidating itself).
That is the pattern I have seen around, I think.

The assumption would then be that creation/assignment is given a reference, I think, but with `Py_NEWREF()` that is OK?
(Or maybe we can make it obvious with methods `.takeref()` and `newref()`, or so, but dunno?)

Code after:
NpyString_release_allocator(npy_string_allocator *allocator);


namespace np { namespace raii {

//
// RAII for PyGILState_* API.
//
// In C++ code, use this at the beginning of a scope, e.g.
//
//     {
//         np::raii::EnsureGIL ensure_gil{};
//         [code that uses the Python C API here]
//     }
//
// instead of
//
//     PyGILState_STATE gil_state =  PyGILState_Ensure();
//     [code that uses the Python C API here]
//     PyGILState_Release(gil_state);
//
// or
//     NPY_ALLOW_C_API_DEF
//     NPY_ALLOW_C_API
//     [code that uses the Python C API here]
//     NPY_DISABLE_C_API
//
// This ensures that PyGILState_Release(gil_state) is called,  even if the
// wrapped code throws an exception or executes a return or a goto.
//
class EnsureGIL
{
    PyGILState_STATE gil_state;

public:

    EnsureGIL() {
        gil_state = PyGILState_Ensure();
    }

    ~EnsureGIL() {
        PyGILState_Release(gil_state);
    }

    EnsureGIL(const EnsureGIL&) = delete;
    EnsureGIL(EnsureGIL&& other) = delete;
    EnsureGIL& operator=(const EnsureGIL&) = delete;
    EnsureGIL& operator=(EnsureGIL&&) = delete;
};
Repository: numpy/numpy
File: numpy/_core/tests/test_dtype.py (Python)

Code before:
            "itemsize": 2
        })
        self.check(Union, expected)

    def test_union_with_struct_packed(self):
        class Struct(ctypes.Structure):
            _pack_ = 1
            _fields_ = [
                ('one', ctypes.c_uint8),
                ('two', ctypes.c_uint32)
            ]

        class Union(ctypes.Union):
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
                ('c', ctypes.c_uint32),
                ('d', Struct),
            ]
        expected = np.dtype({
            "names": ['a', 'b', 'c', 'd'],
            "formats": [
                'u1',
                np.uint16,
                np.uint32,
                [('one', 'u1'), ('two', np.uint32)]
            ],
            "offsets": [0, 0, 0, 0],
            "itemsize": ctypes.sizeof(Union)
        })
        self.check(Union, expected)

    def test_union_packed(self):
        class Struct(ctypes.Structure):
            _fields_ = [
                ('one', ctypes.c_uint8),
                ('two', ctypes.c_uint32)
            ]
            _pack_ = 1

        class Union(ctypes.Union):
            _pack_ = 1
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
                ('c', ctypes.c_uint32),
                ('d', Struct),
            ]
        expected = np.dtype({
            "names": ['a', 'b', 'c', 'd'],
            "formats": [

Review comment (suggestion):
nitpick:

```suggestion
                [('one', 'u1'), ('two', np.uint32)],
```

Code after:
            "itemsize": 2
        })
        self.check(Union, expected)

    def test_union_with_struct_packed(self):
        class Struct(ctypes.Structure):
            _pack_ = 1
            _fields_ = [
                ('one', ctypes.c_uint8),
                ('two', ctypes.c_uint32)
            ]

        class Union(ctypes.Union):
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
                ('c', ctypes.c_uint32),
                ('d', Struct),
            ]
        expected = np.dtype({
            "names": ['a', 'b', 'c', 'd'],
            "formats": [
                'u1',
                np.uint16,
                np.uint32,
                [('one', 'u1'), ('two', np.uint32)],
            ],
            "offsets": [0, 0, 0, 0],
            "itemsize": ctypes.sizeof(Union)
        })
        self.check(Union, expected)

    def test_union_packed(self):
        class Struct(ctypes.Structure):
            _fields_ = [
                ('one', ctypes.c_uint8),
                ('two', ctypes.c_uint32)
            ]
            _pack_ = 1

        class Union(ctypes.Union):
            _pack_ = 1
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
                ('c', ctypes.c_uint32),
                ('d', Struct),
            ]
        expected = np.dtype({
            "names": ['a', 'b', 'c', 'd'],
            "formats": [
Repository: numpy/numpy
File: numpy/_core/tests/test_dtype.py (Python)

Code before:
        })
        self.check(Union, expected)

    def test_union_packed(self):
        class Struct(ctypes.Structure):
            _fields_ = [
                ('one', ctypes.c_uint8),
                ('two', ctypes.c_uint32)
            ]
            _pack_ = 1

        class Union(ctypes.Union):
            _pack_ = 1
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
                ('c', ctypes.c_uint32),
                ('d', Struct),
            ]
        expected = np.dtype({
            "names": ['a', 'b', 'c', 'd'],
            "formats": [
                'u1',
                np.uint16,
                np.uint32,
                [('one', 'u1'), ('two', np.uint32)]
            ],
            "offsets": [0, 0, 0, 0],
            "itemsize": ctypes.sizeof(Union)
        })
        self.check(Union, expected)

    def test_packed_structure(self):
        class PackedStructure(ctypes.Structure):
            _pack_ = 1
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16)
            ]
        expected = np.dtype([
            ('a', np.uint8),
            ('b', np.uint16)
        ])
        self.check(PackedStructure, expected)

    def test_large_packed_structure(self):
        class PackedStructure(ctypes.Structure):
            _pack_ = 2
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),

Review comment (suggestion):
nitpick:

```suggestion
                [('one', 'u1'), ('two', np.uint32)],
```

Code after:
        })
        self.check(Union, expected)

    def test_union_packed(self):
        class Struct(ctypes.Structure):
            _fields_ = [
                ('one', ctypes.c_uint8),
                ('two', ctypes.c_uint32)
            ]
            _pack_ = 1

        class Union(ctypes.Union):
            _pack_ = 1
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
                ('c', ctypes.c_uint32),
                ('d', Struct),
            ]
        expected = np.dtype({
            "names": ['a', 'b', 'c', 'd'],
            "formats": [
                'u1',
                np.uint16,
                np.uint32,
                [('one', 'u1'), ('two', np.uint32)],
            ],
            "offsets": [0, 0, 0, 0],
            "itemsize": ctypes.sizeof(Union)
        })
        self.check(Union, expected)

    def test_packed_structure(self):
        class PackedStructure(ctypes.Structure):
            _pack_ = 1
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16)
            ]
        expected = np.dtype([
            ('a', np.uint8),
            ('b', np.uint16)
        ])
        self.check(PackedStructure, expected)

    def test_large_packed_structure(self):
        class PackedStructure(ctypes.Structure):
            _pack_ = 2
            _fields_ = [
                ('a', ctypes.c_uint8),
                ('b', ctypes.c_uint16),
Repository: numpy/numpy
File: numpy/_core/tests/test_multiprocessing.py (Python)

Code before:
    shm = shared_memory.SharedMemory(name=shm_name)
    arr = np.ndarray(n, dtype=np.bool_, buffer=shm.buf)
    barrier.wait()
    for i in range(n):
        while not arr[i]: pass

def test_read_write_bool_array():
    # See: gh-30389
    #
    # Prior to Python 3.13, boolean scalar singletons (np.True / np.False) were
    # regular reference-counted objects. Due to the double evaluation in
    # PyArrayScalar_RETURN_BOOL_FROM_LONG, concurrent reads and writes of a
    # boolean array could corrupt their refcounts, potentially causing a crash
    # (e.g., `free(): invalid pointer`).
    #
    # This test creates a multi-process race between a writer and a reader to
    # ensure that NumPy does not exhibit such failures.
    barrier = multiprocessing.Barrier(2)
    n = 10000
    shm = shared_memory.SharedMemory(create=True, size=n)
    writer = Process(target=bool_array_writer, args=(barrier, shm.name, n))
    reader = Process(target=bool_array_reader, args=(barrier, shm.name, n))
    writer.start()
    reader.start()
    writer.join()
    reader.join()
    shm.unlink()
    assert writer.exitcode == 0, f"Writer process failed with {writer.exitcode}"
    assert reader.exitcode == 0, f"Reader process failed with {reader.exitcode}"

Review comment (suggestion):
I'd probably write this using `ProcessPoolExecutor` to avoid this boilerplate and join the processes by construction with a `with` statement.

Code after:
    shm = shared_memory.SharedMemory(name=shm_name)
    arr = np.ndarray(n, dtype=np.bool_, buffer=shm.buf)
    for i in range(n):
        while not arr[i]:
            pass

@pytest.mark.skipif(IS_WASM,
                    reason="WASM does not support _posixshmem")
def test_read_write_bool_array():
    # See: gh-30389
    #
    # Prior to Python 3.13, boolean scalar singletons (np.True / np.False) were
    # regular reference-counted objects. Due to the double evaluation in
    # PyArrayScalar_RETURN_BOOL_FROM_LONG, concurrent reads and writes of a
    # boolean array could corrupt their refcounts, potentially causing a crash
    # (e.g., `free(): invalid pointer`).
    #
    # This test creates a multi-process race between a writer and a reader to
    # ensure that NumPy does not exhibit such failures.
    from concurrent.futures import ProcessPoolExecutor
    from multiprocessing import shared_memory
    n = 10000
    shm = shared_memory.SharedMemory(create=True, size=n)
    with ProcessPoolExecutor(max_workers=2) as executor:
        f_writer = executor.submit(bool_array_writer, shm.name, n)
        f_reader = executor.submit(bool_array_reader, shm.name, n)
    shm.unlink()
    f_writer.result()
    f_reader.result()