From b06a6f4f6294dd3550b1a2b6053421d8df145d3c Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Wed, 22 Sep 2021 17:41:56 -0400 Subject: [PATCH] feat: Slice allowing None with py::object or std::optional (#1101) * Adding nullptr slices Using example from #1095 Some fixes from @wjakob's review Stop clang-tidy from complaining New proposal for py::slice constructor Eric's suggested changes: simplify testing; shift def's * chore: drop MSVC pragma (hopefully unneeded) * Apply suggestions from code review --- include/pybind11/detail/common.h | 21 ++++++++++++++++++- include/pybind11/pytypes.h | 25 ++++++++++++++++++++--- include/pybind11/stl.h | 28 ++++++++------------------ tests/test_sequences_and_iterators.cpp | 17 ++++++++++++++++ tests/test_sequences_and_iterators.py | 11 ++++++++++ 5 files changed, 78 insertions(+), 24 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 8aeb79fb7..3bd84da57 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -161,7 +161,26 @@ // https://en.cppreference.com/w/c/chrono/localtime #if defined(__STDC_LIB_EXT1__) && !defined(__STDC_WANT_LIB_EXT1__) -# define __STDC_WANT_LIB_EXT1__ +# define __STDC_WANT_LIB_EXT1__ +#endif + +#ifdef __has_include +// std::optional (but including it in c++14 mode isn't allowed) +# if defined(PYBIND11_CPP17) && __has_include() +# define PYBIND11_HAS_OPTIONAL 1 +# endif +// std::experimental::optional (but not allowed in c++11 mode) +# if defined(PYBIND11_CPP14) && (__has_include() && \ + !__has_include()) +# define PYBIND11_HAS_EXP_OPTIONAL 1 +# endif +// std::variant +# if defined(PYBIND11_CPP17) && __has_include() +# define PYBIND11_HAS_VARIANT 1 +# endif +#elif defined(_MSC_VER) && defined(PYBIND11_CPP17) +# define PYBIND11_HAS_OPTIONAL 1 +# define PYBIND11_HAS_VARIANT 1 #endif #include diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index d1d3dcb05..383663b5e 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -14,6 +14,10 @@ #include #include +#if defined(PYBIND11_HAS_OPTIONAL) +# include +#endif + PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) /* A few forward declarations */ @@ -1345,11 +1349,20 @@ private: class slice : public object { public: PYBIND11_OBJECT_DEFAULT(slice, object, PySlice_Check) - slice(ssize_t start_, ssize_t stop_, ssize_t step_) { - int_ start(start_), stop(stop_), step(step_); + slice(handle start, handle stop, handle step) { m_ptr = PySlice_New(start.ptr(), stop.ptr(), step.ptr()); - if (!m_ptr) pybind11_fail("Could not allocate slice object!"); + if (!m_ptr) + pybind11_fail("Could not allocate slice object!"); } + +#ifdef PYBIND11_HAS_OPTIONAL + slice(std::optional start, std::optional stop, std::optional step) + : slice(index_to_object(start), index_to_object(stop), index_to_object(step)) {} +#else + slice(ssize_t start_, ssize_t stop_, ssize_t step_) + : slice(int_(start_), int_(stop_), int_(step_)) {} +#endif + bool compute(size_t length, size_t *start, size_t *stop, size_t *step, size_t *slicelength) const { return PySlice_GetIndicesEx((PYBIND11_SLICE_OBJECT *) m_ptr, @@ -1364,6 +1377,12 @@ public: stop, step, slicelength) == 0; } + +private: + template + static object index_to_object(T index) { + return index ? object(int_(*index)) : object(none()); + } }; class capsule : public object { diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 99b49d0e2..2c017b4fe 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -9,6 +9,7 @@ #pragma once +#include "detail/common.h" #include "pybind11.h" #include #include @@ -19,28 +20,15 @@ #include #include -#ifdef __has_include -// std::optional (but including it in c++14 mode isn't allowed) -# if defined(PYBIND11_CPP17) && __has_include() -# include -# define PYBIND11_HAS_OPTIONAL 1 -# endif -// std::experimental::optional (but not allowed in c++11 mode) -# if defined(PYBIND11_CPP14) && (__has_include() && \ - !__has_include()) -# include -# define PYBIND11_HAS_EXP_OPTIONAL 1 -# endif -// std::variant -# if defined(PYBIND11_CPP17) && __has_include() -# include -# define PYBIND11_HAS_VARIANT 1 -# endif -#elif defined(_MSC_VER) && defined(PYBIND11_CPP17) +// See `detail/common.h` for implementation of these guards. +#if defined(PYBIND11_HAS_OPTIONAL) # include +#elif defined(PYBIND11_HAS_EXP_OPTIONAL) +# include +#endif + +#if defined(PYBIND11_HAS_VARIANT) # include -# define PYBIND11_HAS_OPTIONAL 1 -# define PYBIND11_HAS_VARIANT 1 #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index 66d647262..72d96cb44 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -17,6 +17,11 @@ #include #include +#ifdef PYBIND11_HAS_OPTIONAL +#include +#endif // PYBIND11_HAS_OPTIONAL + + template class NonZeroIterator { const T* ptr_; @@ -117,6 +122,18 @@ TEST_SUBMODULE(sequences_and_iterators, m) { return std::make_tuple(istart, istop, istep); }); + m.def("make_forward_slice_size_t", []() { return py::slice(0, -1, 1); }); + m.def("make_reversed_slice_object", []() { return py::slice(py::none(), py::none(), py::int_(-1)); }); +#ifdef PYBIND11_HAS_OPTIONAL + m.attr("has_optional") = true; + m.def("make_reversed_slice_size_t_optional_verbose", []() { return py::slice(std::nullopt, std::nullopt, -1); }); + // Warning: The following spelling may still compile if optional<> is not present and give wrong answers. + // Please use with caution. + m.def("make_reversed_slice_size_t_optional", []() { return py::slice({}, {}, -1); }); +#else + m.attr("has_optional") = false; +#endif + // test_sequence class Sequence { public: diff --git a/tests/test_sequences_and_iterators.py b/tests/test_sequences_and_iterators.py index 2c73eff29..79689391a 100644 --- a/tests/test_sequences_and_iterators.py +++ b/tests/test_sequences_and_iterators.py @@ -16,6 +16,17 @@ def allclose(a_list, b_list, rel_tol=1e-05, abs_tol=0.0): ) +def test_slice_constructors(): + assert m.make_forward_slice_size_t() == slice(0, -1, 1) + assert m.make_reversed_slice_object() == slice(None, None, -1) + + +@pytest.mark.skipif(not m.has_optional, reason="no ") +def test_slice_constructors_explicit_optional(): + assert m.make_reversed_slice_size_t_optional() == slice(None, None, -1) + assert m.make_reversed_slice_size_t_optional_verbose() == slice(None, None, -1) + + def test_generalized_iterators(): assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).nonzero()) == [(1, 2), (3, 4)] assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero()) == [(1, 2)]