add support for const-only smart pointers

[Rosdf]

Description

In my project, I am mainly using smart pointers that can hold only pointers to const, but Pybind does not allow them to be passed to Python. Here is a small fix to allow this behavior

Suggested changelog entry:

• Placeholder.

[Rosdf]

If idea is ok, I will fix failing builds

[rwgk]

TL;DR:

• The production code changes are small and seem fine at first sight.

• But your use case leaks. Could you please provide a more convincing use case?

---

To explain to myself(!): Why do we need the production code changes?

We have existing tests for std::shared_ptr<const T>:

• pybind11/tests/test_class_sh_basic.cpp

  Line 82 in 6c5d25a

  std::shared_ptr<atyp const> rtrn_shcp() { return std::shared_ptr<atyp const>(new atyp{"rtrn_shcp"}); }

However, const-ness of T is cast away (look for // Const2Mutbl comments), the wrapped object is still a std::shared_ptr<T>, which has (e.g.) operator * (non-const).

In contrast, your StaticPtr only has const T member functions. That's apparently why you need a const-cast somewhere else (your diff):

     template <typename V = void>
     V *&value_ptr() const {
-        return reinterpret_cast<V *&>(vh[0]);
+        if constexpr (std::is_const_v<V>) {
+            return reinterpret_cast<V *&>(const_cast<const void *&>(vh[0]));
+        } else {
+            return reinterpret_cast<V *&>(vh[0]);
+        }
     }

I think that'd be OK (with a C++11-compatible implementation) ... if you can provide a convincing use case:

Looking at your test code, I believe this leaks:

        return StaticPtr(new MyData(std::move(name)));

I don't see a corresponding delete anywhere. Could you please explain?

Could you please provide a more realistic use case?

[Rosdf]

To be honest, this my real use case. I have some number of objects, that are allocate and are alive until program exits, so for them I use StaticPtr wrapper, and they are const because i share them across threads

[rwgk]

First impression: That's a pretty involved way of dealing with the situation.

Are you aware of this?

• https://pybind11.readthedocs.io/en/stable/advanced/classes.html#non-public-destructors

I tested this locally:

#include "pybind11_tests.h"

#include <string>

class MyData {
public:
    static std::unique_ptr<MyData, py::nodelete> create(std::string name) {
        return std::unique_ptr<MyData, py::nodelete>(new MyData(std::move(name)));
    }

    const std::string &getName() const { return name_; }

private:
    explicit MyData(std::string &&name) : name_(std::move(name)) {}

    std::string name_;
};

TEST_SUBMODULE(const_module, m) {
    py::class_<MyData, std::unique_ptr<MyData, py::nodelete>>(m, "Data")
        .def(py::init([](const std::string &name) { return MyData::create(name); }))
        .def_property_readonly("name", &MyData::getName);
}

Could that pattern work for your real use case?

[rwgk]

I forgot to add, you could just copy this into your codebase, under your own namespace, if you want keep your C++ code independent from pybind11:

struct nodelete {                                                               
    template <typename T>                                                       
    void operator()(T *) {}                                                     
};                                                                              

[Rosdf]

Using unique_ptr in my code base would not work because it is not copyable.

I can do something like this

class MyData {
public:
    static StaticPtr<MyData> getOrCreate(const std::string &name) {
        // here should be logic to get or create a pointer
        const MyData *ptr = new MyData(std::string(name));

        return StaticPtr(ptr);
    }

    const std::string &getName() const { return name_; }

private:
    explicit MyData(std::string &&name) : name_(std::move(name)) {}

    std::string name_;
};

TEST_SUBMODULE(const_module, m) {
    py::class_<MyData, std::unique_ptr<MyData, py::nodelete>>(m, "Data")
        .def(py::init([](const std::string &name) {
            auto ptr = MyData::getOrCreate(name);
            return std::unique_ptr<MyData, py::nodelete>(const_cast<MyData *>(ptr.get()));
        }))
        .def_property_readonly("name", &MyData::getName);
}

but then i won't be able to easily export some functions that use StaticPtr<MyData>

[rwgk]

I tested this locally:

#include "pybind11_tests.h"

#include <string>

class MyData {
public:
    static std::shared_ptr<MyData> create(std::string name) {
        return std::shared_ptr<MyData>(new MyData(std::move(name)), [](MyData *) {});
    }

    const std::string &getName() const { return name_; }

private:
    explicit MyData(std::string &&name) : name_(std::move(name)) {}

    std::string name_;
};

TEST_SUBMODULE(const_module, m) {
    py::class_<MyData, py::smart_holder>(m, "Data")
        .def(py::init([](const std::string &name) { return MyData::create(name); }))
        .def_property_readonly("name", &MyData::getName);
}

The more I think about this, the more doubts I have about that pattern.

What's the purpose of a factory function that always leaks? Make it unobvious that you're leaking memory?

[Rosdf]

It always leaks just for this example; in reality, it first searches for existing pointer associated with this name, if it fails, creates new one.

Using shared_pointer in my code base is also not an option, because it does extra ref counting and takes twice the space.

If it will be more convincing, let's imagine that my use case is this.

template <class T>
class MySmartPointer {
public:
    // regular shared_ptr methods

    const T * get() const { return data_; }

private:
    const T * data_ = nullptr;
    std::atomic<uint32_t> * counter_ = nullptr; 
}

[rwgk]

Using shared_pointer in my code base is also not an option, because it does extra ref counting and takes twice the space.

I want to look in two directions then:

1. Make this PR work on all platforms.

2. Apart from optimization considerations ("extra ref counting and takes twice the space"), could this work just for you:

Almost identical, just a couple const removed:

template <class T>
class MySmartPointer {
public:
    // regular shared_ptr methods

   /*const*/ T * get() const { return data_; }

private:
    /*const*/ T * data_ = nullptr;
    std::atomic<uint32_t> * counter_ = nullptr; 
}

Then: MySmartPointer<const MyData> (similar to std::shared_ptr<const MyData>).

[rwgk]

Could we do this unconditionally (without the std::is_const_v<V> test)?

To keep the code simpler.

[Rosdf]

no it will cause errors like this error: ‘reinterpret_cast’ from type ‘const void**’ to type ‘void**’ casts away qualifiers

[rwgk]

I just pushed fc8ca60

Copying the link from there: https://claude.ai/share/4085d9ab-a859-44cc-bb56-450e472f817a

[rwgk]

Assuming the CI still passes, I'm fine with the production code changes.

However, we need a test case that does not leak. I wouldn't want to surprise someone (years) later who's trying to use this test for leak detection while working on a refactoring, and is plausibly unaware that the test code has leaks. It's also possible that future tooling will flag this code.

[rwgk]

For completeness, I also quizzed ChatGPT:

• https://chatgpt.com/share/6847137e-33ec-8008-a1fb-05be144a3f2e

I still have doubts about your StaticPtr design, but I'm fine with the production code changes.

Could you please rename StaticPtr to ConstSmartPtr (in addition to make the test non-leaking)? Please use a namespace in the test (e.g. namespace test_const_smart_ptr), to guard against name clashes (e.g. someone else might try to use MyData elsewhere and then get surprised by ODR violations).

[rwgk]

I think test_const_only_smart_ptr would be a better name, to clearly distinguish it from e.g. std::shard_ptr<const MyData>.

[rwgk]

You can put

using namespace const_only_smart_ptr;

here. That's local to this translation unit (no danger of name clashes). The only important part is that the definitions are inside a namespace.

I tried that locally, to be sure what I'm writing actually works, then pushed it.

It's now test_const_only_smart_ptr, which is what we usually have here.

[rwgk]

ChatGPT is suggesting the below. Could you please adapt?

Here is the full conversation (which also explains why I think it's good to go the extra mile, even though it's only a test): https://chatgpt.com/share/684768ba-fc1c-8008-bdbf-f31844b3ec4d

template <class T>
class non_sync_const_shared_ptr {
public:
    non_sync_const_shared_ptr() = default;
    explicit non_sync_const_shared_ptr(T *ptr) : ptr_(ptr) {
        if (ptr_) {
            ref_count_ = new std::size_t(1);
        }
    }
    non_sync_const_shared_ptr(const non_sync_const_shared_ptr &other)
        : ptr_(other.ptr_), ref_count_(other.ref_count_) {
        if (ref_count_) {
            ++(*ref_count_);
        }
    }
    non_sync_const_shared_ptr &operator=(const non_sync_const_shared_ptr &other) {
        if (this != &other) {
            release();
            ptr_ = other.ptr_;
            ref_count_ = other.ref_count_;
            if (ref_count_) {
                ++(*ref_count_);
            }
        }
        return *this;
    }
    ~non_sync_const_shared_ptr() {
        release();
    }

    const T *get() const { return ptr_; }
    const T &operator*() const { return *ptr_; }
    const T *operator->() const { return ptr_; }
    explicit operator bool() const { return ptr_ != nullptr; }

private:
    void release() {
        if (ref_count_) {
            --(*ref_count_);
            if (*ref_count_ == 0) {
                delete ptr_;
                delete ref_count_;
            }
        }
    }

    const T *ptr_ = nullptr;
    std::size_t *ref_count_ = nullptr;
};

[rwgk]

claude.ai has it's own opinion and suggested fixes:

https://claude.ai/share/8eaaf0d9-04ba-48cd-a858-ac0f13ce97bb

I'll stop here. I hope it's pretty easy for you to pick out what's valuable.

[rwgk]

Sorry, the test is getting way too big now (partially my fault).

Offline @henryiii suggested simply adding something like a minimal const_only_smart_ptr to the existing test_smart_ptr.cpp,py. I think that'll be best in this case.