Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[smart_holder] Bug fix: std::unique_ptr deleter needs to be copied. #4850

Merged
merged 16 commits into from
Nov 2, 2023

Commits on Sep 19, 2023

  1. Store std::function<void (void *)> del_fun; in guarded_delete

    Ralf W. Grosse-Kunstleve committed Sep 19, 2023
    Configuration menu
    Copy the full SHA
    f14846d View commit details
    Browse the repository at this point in the history
  2. Specialize the simple common case.

    Using a `union` is complicated: https://en.cppreference.com/w/cpp/language/union
    
    > If members of a union are classes with user-defined constructors and destructors, to switch the active member, explicit destructor and placement new are generally needed:
    
    Using `std::variant` increases compile-time overhead.
    
    It is currently unclear how much these effects matter in practice: optimization left for later.
    Ralf W. Grosse-Kunstleve committed Sep 19, 2023
    Configuration menu
    Copy the full SHA
    24bf40b View commit details
    Browse the repository at this point in the history
  3. Add one test case (more later).

    Ralf W. Grosse-Kunstleve committed Sep 19, 2023
    Configuration menu
    Copy the full SHA
    884fe1c View commit details
    Browse the repository at this point in the history
  4. Add const to resolve clang-tidy error.

    ```
    -- The CXX compiler identification is Clang 15.0.7
    
    /usr/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy;--use-color;--warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/__w/pybind11/pybind11/tests/test_class_sh_inheritance.cpp -- /usr/bin/c++ -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/__w/pybind11/pybind11/include -isystem /usr/include/python3.9 -isystem /__w/pybind11/pybind11/build/_deps/eigen-src -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -flto=thin -std=c++17 -o CMakeFiles/pybind11_tests.dir/test_class_sh_inheritance.cpp.o -c /__w/pybind11/pybind11/tests/test_class_sh_inheritance.cpp
    /__w/pybind11/pybind11/tests/pure_cpp/smart_holder_poc_test.cpp:264:30: error: pointer parameter 'raw_ptr' can be pointer to const [readability-non-const-parameter,-warnings-as-errors]
            new int(19), [](int *raw_ptr) { delete raw_ptr; });
                                 ^
                            const
    ```
    Ralf W. Grosse-Kunstleve committed Sep 19, 2023
    Configuration menu
    Copy the full SHA
    435d386 View commit details
    Browse the repository at this point in the history

Commits on Sep 22, 2023

  1. Introduce struct custom_deleter to ensure the deleter is moved as d…

    …esired (the lambda function only captures a reference, which can become dangling).
    Ralf W. Grosse-Kunstleve committed Sep 22, 2023
    Configuration menu
    Copy the full SHA
    17098eb View commit details
    Browse the repository at this point in the history
  2. Resolve helpful clang-tidy errors.

    ```
    /usr/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy;--use-color;--warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/__w/pybind11/pybind11/tests/test_class.cpp -- /usr/bin/c++ -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/__w/pybind11/pybind11/include -isystem /usr/include/python3.9 -isystem /__w/pybind11/pybind11/build/_deps/eigen-src -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -flto=thin -std=c++17 -o CMakeFiles/pybind11_tests.dir/test_class.cpp.o -c /__w/pybind11/pybind11/tests/test_class.cpp
    /__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:114:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors]
        custom_deleter(D &&deleter) : deleter{std::move(deleter)} {}
        ^
        explicit
    /__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:120:76: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
        return guarded_delete(std::function<void(void *)>(custom_deleter<T, D>(std::move(uqp_del))),
                                                                               ^~~~~~~~~
                                                                               std::forward<D>
    ```
    Ralf W. Grosse-Kunstleve committed Sep 22, 2023
    Configuration menu
    Copy the full SHA
    6083f8a View commit details
    Browse the repository at this point in the history
  3. Workaround for gcc 4.8.5, clang 3.6

    Ralf W. Grosse-Kunstleve committed Sep 22, 2023
    Configuration menu
    Copy the full SHA
    d815d7d View commit details
    Browse the repository at this point in the history

Commits on Nov 1, 2023

  1. Merge branch 'smart_holder' into unique_ptr_deleter_sh

    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    4dbe657 View commit details
    Browse the repository at this point in the history
  2. Transfer reduced test here.

    Reduced from a PyCLIF use case in the wild by @wangxf123456 (internal change cl/565476030).
    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    c684f6c View commit details
    Browse the repository at this point in the history
  3. Add missing include (clangd Include Cleaner)

    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    a685d57 View commit details
    Browse the repository at this point in the history
  4. Change std::move to std::forward as suggested by @iwanders.

    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    8a973d4 View commit details
    Browse the repository at this point in the history
  5. Add missing includes (clangd Include Cleaner)

    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    4fb7d12 View commit details
    Browse the repository at this point in the history
  6. Use new PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP to exclud…

    …e `smart_holder::as_unique_ptr` method from production code.
    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    4d9d722 View commit details
    Browse the repository at this point in the history
  7. Systematically add `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP…

    …` to mark code that is not used from production code. Add comment to explain.
    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    7669a5b View commit details
    Browse the repository at this point in the history
  8. Very simple experiment related to pybind#4850 (comment)

    Does the `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` define have anything to do with it?
    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    fe59369 View commit details
    Browse the repository at this point in the history
  9. Revert "Very simple experiment related to pybind#4850 (comment)"

    This reverts commit fe59369.
    Ralf W. Grosse-Kunstleve committed Nov 1, 2023
    Configuration menu
    Copy the full SHA
    821e13a View commit details
    Browse the repository at this point in the history