fix: Let an owning null handle be copied, and build as C++20

.github/workflows/ci.yml

@@ -11,44 +11,35 @@ on:
   workflow_dispatch:
 
 jobs:
-  test_debug:
+  test:
     runs-on: ubuntu-latest
     permissions:
       contents: read
-    container:
-      image: tschijnmo/drudge:base
-
-    steps:
-      - name: Checkout repository with submodules
-        uses: actions/checkout@v3
-        with:
-          submodules: recursive
-
-      - name: Build and test (debug)
-        run: |
-          mkdir cmake-test-debug
-          cd cmake-test-debug
-          cmake -DCMAKE_BUILD_TYPE=debug ..
-          make
-          ./test/testmain
+    strategy:
+      matrix:
+        build_type: [Debug, Release]
 
-  test_release:
-    runs-on: ubuntu-latest
-    permissions:
-      contents: read
-    container:
-      image: tschijnmo/drudge:base
-
     steps:
-      - name: Checkout repository with submodules
-        uses: actions/checkout@v3
-        with:
-          submodules: recursive
-
-      - name: Build and test (release)
-        run: |
-          mkdir cmake-test-release
-          cd cmake-test-release
-          cmake -DCMAKE_BUILD_TYPE=release ..
-          make
-          ./test/testmain
+    - name: Checkout code
+      uses: actions/checkout@v6
+
+    - name: Install dependencies
+      run: |
+        sudo apt-get update
+        sudo apt-get install -y cmake g++ python3-dev
+
+    - name: Configure CMake
+      run: |
+        mkdir build
+        cd build
+        cmake -DCMAKE_BUILD_TYPE=${{ matrix.build_type }} ..
+
+    - name: Build
+      run: |
+        cd build
+        make
+
+    - name: Run tests
+      run: |
+        cd build
+        ./test/testmain

CMakeLists.txt

@@ -1,20 +1,21 @@
-cmake_minimum_required(VERSION 3.6)
+cmake_minimum_required(VERSION 3.18)
 project(cpypp VERSION 0.1.0 LANGUAGES CXX)
 
 # OPTIONS
 option(BUILD_TESTS "Build unit tests" ON)
 
 # Set the building options.
-set(CMAKE_CXX_STANDARD 14)
+set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_CXX_EXTENSIONS OFF)
 
 # Find the Python runtime.
-include(FindPythonLibs)
+find_package(Python3 REQUIRED COMPONENTS Development.Embed)
 message(STATUS
-    "Find Python header at ${PYTHON_INCLUDE_DIRS}\nand library at ${PYTHON_LIBRARIES}"
+    "Find Python header at ${Python3_INCLUDE_DIRS}\nand library at ${Python3_LIBRARIES}"
 )
 
 if (BUILD_TESTS)
     enable_testing()
     add_subdirectory(test)
 endif ()
-

include/cpypp.hpp

@@ -636,13 +636,17 @@ class Handle {
         }
     }
 
-    /** Increments the reference count only for owning reference.
+    /** Increments the reference count for an owning reference, if not null.
+     *
+     * An owning handle can hold a null pointer, for instance the value of an
+     * exhausted `Iter_handle`, so null is tolerated here just as it is in
+     * `decr_ref`.
      */
 
     void incr_ref() noexcept
     {
         if (!if_borrow_) {
-            Py_INCREF(ref_);
+            Py_XINCREF(ref_);
         }
     }
 

test/CMakeLists.txt

@@ -1,10 +1,14 @@
-# Download the Catch2 single header.
+# Fetch Catch2 v3.11.0
+include(FetchContent)
 
-file(DOWNLOAD
-    "https://github.com/catchorg/Catch2/releases/download/v2.0.1/catch.hpp"
-    "${CMAKE_CURRENT_BINARY_DIR}/catch.hpp"
+FetchContent_Declare(
+  Catch2
+  GIT_REPOSITORY https://github.com/catchorg/Catch2.git
+  GIT_TAG        v3.11.0
 )
 
+FetchContent_MakeAvailable(Catch2)
+
 # The main test driver.
 add_executable(testmain
     testmain.cpp
@@ -21,11 +25,9 @@ add_executable(testmain
 )
 
 target_include_directories(testmain
-    PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
     PRIVATE "${PROJECT_SOURCE_DIR}/include"
-    PRIVATE ${PYTHON_INCLUDE_DIRS}
 )
 target_link_libraries(testmain
-    PRIVATE ${PYTHON_LIBRARIES}
+    PRIVATE Catch2::Catch2
+    PRIVATE Python3::Python
 )
-

test/exc.cpp

@@ -1,7 +1,7 @@
 /** Tests for the utilities related to Python exception handling.
  */
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 

test/fundamentalobjects.cpp

@@ -1,7 +1,7 @@
 /** Tests for the utility for fundamental objects.
  */
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 
@@ -77,7 +77,11 @@ TEST_CASE("Trivial static type has basic features", "[Handle][Static_type]")
             auto handle = noddy_type.get_handle(true);
             CHECK(handle.get() == noddy_ptr);
             CHECK(!handle.if_borrow());
-            CHECK(Py_REFCNT(noddy_ptr) == init_count + 1);
+            // Static types are immortal from Python 3.12 on, and then their
+            // reference count does not move.
+            if (init_count < (static_cast<Py_ssize_t>(1) << 30)) {
+                CHECK(Py_REFCNT(noddy_ptr) == init_count + 1);
+            }
         }
         CHECK(Py_REFCNT(noddy_ptr) == init_count);
     }

test/handle.cpp

@@ -11,7 +11,7 @@
 
 #include <memory>
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 
@@ -21,17 +21,18 @@ using namespace cpypp;
 
 TEST_CASE("Owning handles correctly manages reference counts", "[Handle]")
 {
-    PyObject* one = Py_BuildValue("i", 1);
+    // Large integers are used, since small integers are cached by CPython
+    // and immortal from Python 3.12 on, where their reference count never
+    // moves.  These two are fresh objects whose count we fully control.
+    PyObject* one = Py_BuildValue("L", static_cast<long long>(1) << 40);
     // Py_BuildValue gives a new reference to be possibly stolen by
     // Handles.  Here we increment the reference count again to make sure
     // that even after that reference is destroyed, we still have at least
-    // one reference.  Practically this is unnecessary in that the integer
-    // unity normally has hundreds of references to prevent it from being
-    // collected.
+    // one reference.
     Py_INCREF(one);
     Py_ssize_t init_count = Py_REFCNT(one);
 
-    PyObject* two = Py_BuildValue("i", 2);
+    PyObject* two = Py_BuildValue("L", static_cast<long long>(1) << 41);
     Py_INCREF(two); // For the same rational as one.
     Py_ssize_t init_count2 = Py_REFCNT(two);
 
@@ -351,8 +352,8 @@ TEST_CASE("Borrowing handles correctly treats reference counts", "[Handle]")
 {
     // Testing of borrowing handles is relatively easy, no matter what
     // happens, the reference count should never be touched.
-    PyObject* one = Py_BuildValue("i", 1);
-    PyObject* two = Py_BuildValue("i", 2);
+    PyObject* one = Py_BuildValue("L", static_cast<long long>(1) << 40);
+    PyObject* two = Py_BuildValue("L", static_cast<long long>(1) << 41);
     Py_ssize_t init_count = Py_REFCNT(one);
     Py_ssize_t init_count2 = Py_REFCNT(two);
     auto check_ref = [&]() {

test/iteratorprotocol.cpp

@@ -5,7 +5,7 @@
 #include <iterator>
 #include <vector>
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 
@@ -54,6 +54,37 @@ TEST_CASE("Handles has basic iterator protocol", "[Handle][Iter_handle]")
     }
 }
 
+TEST_CASE("Iterators over empty iterables can be copied", "[Handle][Iter_handle]")
+{
+    // An exhausted iterator holds an owning null handle as its value.  Copying
+    // it used to increment the reference count of the null pointer.
+    Handle empty("[]");
+
+    SECTION("an exhausted iterator can be copied")
+    {
+        auto it = empty.begin();
+        check_exc();
+        REQUIRE_FALSE(it.has_val());
+        Iter_handle copy(it);
+        CHECK_FALSE(copy.has_val());
+        CHECK(copy == empty.end());
+    }
+
+    SECTION("a container can be built from an empty iterable")
+    {
+        std::vector<Handle> res(empty.begin(), empty.end());
+        check_exc();
+        CHECK(res.empty());
+    }
+
+    SECTION("an owning null handle can be copied")
+    {
+        Handle null(nullptr, STEAL, true);
+        Handle copy(null);
+        CHECK_FALSE(copy);
+    }
+}
+
 TEST_CASE("Iterator utilities report wrong types", "[Handle][Iter_handle]")
 {
     // A non-iterable and non-iterator integer.

test/numberprotocol.cpp

@@ -3,7 +3,7 @@
 
 #include <functional>
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 

test/numericobjects.cpp

@@ -1,14 +1,21 @@
 /** Tests for the utilities to related numeric objects.
  */
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 
 #include <cpypp.hpp>
 
 using namespace cpypp;
 
+// Small integers are immortal from Python 3.12 on, and the reference count of
+// an immortal object does not move.  Their count is only checked when it can.
+static bool count_moves(PyObject* obj)
+{
+    return Py_REFCNT(obj) < (static_cast<Py_ssize_t>(1) << 30);
+}
+
 TEST_CASE("Integer can be built and parsed", "[Handle]")
 {
 
@@ -22,7 +29,9 @@ TEST_CASE("Integer can be built and parsed", "[Handle]")
         {
             Handle from_gen(v);
             CHECK(from_gen.get() == one);
-            CHECK(Py_REFCNT(one) == curr_count + 1);
+            if (count_moves(one)) {
+                CHECK(Py_REFCNT(one) == curr_count + 1);
+            }
 
             long val;
             from_gen.as(val);

test/objectprotocol.cpp

@@ -3,7 +3,7 @@
 
 #include <memory>
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 

test/otherobjects.cpp

@@ -1,7 +1,7 @@
 /** Tests for utility for uncategorized concrete object.
  */
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 

test/parsebuild.cpp

@@ -5,14 +5,21 @@
  * objects.
  */
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 
 #include <cpypp.hpp>
 
 using namespace cpypp;
 
+// Small integers are immortal from Python 3.12 on, and the reference count of
+// an immortal object does not move.  Their count is only checked when it can.
+static bool count_moves(PyObject* obj)
+{
+    return Py_REFCNT(obj) < (static_cast<Py_ssize_t>(1) << 30);
+}
+
 TEST_CASE("General utility can make and parse simple integers", "[Handle]")
 {
 
@@ -26,7 +33,9 @@ TEST_CASE("General utility can make and parse simple integers", "[Handle]")
     {
         Handle from_gen("i", 1);
         CHECK(from_gen.get() == one);
-        CHECK(Py_REFCNT(one) == curr_count + 1);
+        if (count_moves(one)) {
+            CHECK(Py_REFCNT(one) == curr_count + 1);
+        }
 
         long val;
         from_gen.as(val);

test/sequenceobjects.cpp

@@ -1,7 +1,7 @@
 /** Tests for the utility for sequence objects.
  */
 
-#include <catch.hpp>
+#include <catch2/catch_test_macros.hpp>
 
 #include <Python.h>
 

test/testmain.cpp

@@ -1,5 +1,4 @@
-#define CATCH_CONFIG_RUNNER
-#include <catch.hpp>
+#include <catch2/catch_session.hpp>
 
 #include <Python.h>