From 70dfa81423bd99f8959ae0d80f81234a40d32e6e Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 19 Feb 2024 21:17:47 +0100 Subject: [PATCH 1/3] Rule of 5 for Converter class A class with a virtual destructor should also define copying and assignment. In the case of the converter class, both should be forbidden. This would have helped me understanding the code if it would have been done before. --- src/Converters.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Converters.h b/src/Converters.h index bfba9758..80b1d0c3 100644 --- a/src/Converters.h +++ b/src/Converters.h @@ -17,6 +17,13 @@ class CPYCPPYY_CLASS_EXPORT Converter { public: virtual ~Converter(); + Converter() = default; + + Converter(Converter const& other) = delete; + Converter(Converter && other) = delete; + Converter& operator=(Converter const& other) = delete; + Converter& operator=(Converter && other) = delete; + public: virtual bool SetArg(PyObject*, Parameter&, CallContext* = nullptr) = 0; virtual PyObject* FromMemory(void* address); From e3011c397fd394d7466b5522101ed7a44a3e073e Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 19 Feb 2024 17:46:09 +0100 Subject: [PATCH 2/3] Carry a copy of the string also in the `std::string_view` converter There are two possible fixes to the problem with string lifetimes and `std::string_view` arguments: * setting a lifeline * copying the string Copying the string is supposedly faster on average, at least in the ROOT usecase the strings that are passed around are not very long (RDataFrame filter and variable definitions). This commit fixes the following reproducer: ```Python import cppyy cppyy.cppdef(""" void foo(std::string_view s1, std::string_view s2) { std::cout << s1 << std::endl; std::cout << s2 << std::endl; std::cout << std::endl; } void bar(std::initializer_list sl) { for (auto const& s : sl) { std::cout << s << std::endl; } std::cout << std::endl; } """) cppyy.gbl.foo("hello", "world") ``` Closes #13. --- src/Converters.cxx | 36 ++++++++++++++++++++++++++++-------- src/DeclareConverters.h | 11 ++++++++--- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/Converters.cxx b/src/Converters.cxx index 661dbf52..760889c3 100644 --- a/src/Converters.cxx +++ b/src/Converters.cxx @@ -1833,8 +1833,8 @@ CPyCppyy::name##Converter::name##Converter(bool keepControl) : \ bool CPyCppyy::name##Converter::SetArg( \ PyObject* pyobject, Parameter& para, CallContext* ctxt) \ { \ - if (CPyCppyy_PyUnicodeAsBytes2Buffer(pyobject, fBuffer)) { \ - para.fValue.fVoidp = &fBuffer; \ + if (CPyCppyy_PyUnicodeAsBytes2Buffer(pyobject, fStringBuffer)) { \ + para.fValue.fVoidp = &fStringBuffer; \ para.fTypeCode = 'V'; \ return true; \ } \ @@ -1871,8 +1871,14 @@ CPPYY_IMPL_STRING_AS_PRIMITIVE_CONVERTER(STLStringViewBase, std::string_view, da bool CPyCppyy::STLStringViewConverter::SetArg( PyObject* pyobject, Parameter& para, CallContext* ctxt) { - if (this->STLStringViewBaseConverter::SetArg(pyobject, para, ctxt)) + if (this->STLStringViewBaseConverter::SetArg(pyobject, para, ctxt)) { + // One extra step compared to the regular std::string converter: + // Create a corresponding std::string_view and set the parameter value + // accordingly. + fStringView = *reinterpret_cast(para.fValue.fVoidp); + para.fValue.fVoidp = &fStringView; return true; + } if (!CPPInstance_Check(pyobject)) return false; @@ -1884,14 +1890,28 @@ bool CPyCppyy::STLStringViewConverter::SetArg( if (!ptr) return false; - fBuffer = *((std::string*)ptr); - para.fValue.fVoidp = &fBuffer; + // Copy the string to ensure the lifetime of the string_view and the + // underlying string is identical. + fStringBuffer = *((std::string*)ptr); + // Create the string_view on the copy + fStringView = fStringBuffer; + para.fValue.fVoidp = &fStringView; para.fTypeCode = 'V'; return true; } return false; } +bool CPyCppyy::STLStringViewConverter::ToMemory( + PyObject* value, void* address, PyObject* ctxt) +{ + if (CPyCppyy_PyUnicodeAsBytes2Buffer(value, fStringBuffer)) { + fStringView = fStringBuffer; + *reinterpret_cast(address) = fStringView; + return true; + } + return InstanceConverter::ToMemory(value, address, ctxt); +} #endif CPyCppyy::STLWStringConverter::STLWStringConverter(bool keepControl) : @@ -1902,9 +1922,9 @@ bool CPyCppyy::STLWStringConverter::SetArg( { if (PyUnicode_Check(pyobject)) { Py_ssize_t len = CPyCppyy_PyUnicode_GET_SIZE(pyobject); - fBuffer.resize(len); - CPyCppyy_PyUnicode_AsWideChar(pyobject, &fBuffer[0], len); - para.fValue.fVoidp = &fBuffer; + fStringBuffer.resize(len); + CPyCppyy_PyUnicode_AsWideChar(pyobject, &fStringBuffer[0], len); + para.fValue.fVoidp = &fStringBuffer; para.fTypeCode = 'V'; return true; } diff --git a/src/DeclareConverters.h b/src/DeclareConverters.h index 9e4c2b35..6d8acaee 100644 --- a/src/DeclareConverters.h +++ b/src/DeclareConverters.h @@ -351,21 +351,26 @@ CPPYY_DECLARE_BASIC_CONVERTER(PyObject); class name##Converter : public InstanceConverter { \ public: \ name##Converter(bool keepControl = true); \ -public: \ virtual bool SetArg(PyObject*, Parameter&, CallContext* = nullptr); \ virtual PyObject* FromMemory(void* address); \ virtual bool ToMemory(PyObject*, void*, PyObject* = nullptr); \ virtual bool HasState() { return true; } \ protected: \ - strtype fBuffer; \ + strtype fStringBuffer; \ } CPPYY_DECLARE_STRING_CONVERTER(STLString, std::string); #if __cplusplus > 201402L -CPPYY_DECLARE_STRING_CONVERTER(STLStringViewBase, std::string_view); +// The buffer type needs to be std::string also in the string_view case, +// otherwise the pointed-to string might not live long enough. See also: +// https://github.com/wlav/CPyCppyy/issues/13 +CPPYY_DECLARE_STRING_CONVERTER(STLStringViewBase, std::string); class STLStringViewConverter : public STLStringViewBaseConverter { public: virtual bool SetArg(PyObject*, Parameter&, CallContext* = nullptr); + virtual bool ToMemory(PyObject*, void*, PyObject* = nullptr); +private: + std::string_view fStringView; }; #endif CPPYY_DECLARE_STRING_CONVERTER(STLWString, std::wstring); From 8b7145f45c3e713eadc9231917e7e58202a9432e Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 19 Feb 2024 21:23:19 +0100 Subject: [PATCH 3/3] Separate converters for each element in initializer list converter If they have state, a separate converter needs to be created for each element in the initializer list. Fixes the following reproducer: ```python import cppyy cppyy.cppdef(""" void foo(std::string_view s1, std::string_view s2) { std::cout << s1 << std::endl; std::cout << s2 << std::endl; std::cout << std::endl; } void bar(std::initializer_list sl) { for (auto const& s : sl) { std::cout << s << std::endl; } std::cout << std::endl; } """) cppyy.gbl.foo("hello", "world") cppyy.gbl.bar(["hello", "world"]) ``` --- src/Converters.cxx | 24 +++++++++++++++++++----- src/DeclareConverters.h | 9 ++++----- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/Converters.cxx b/src/Converters.cxx index 760889c3..dac0857d 100644 --- a/src/Converters.cxx +++ b/src/Converters.cxx @@ -2872,9 +2872,20 @@ struct faux_initlist } // unnamed namespace +CPyCppyy::InitializerListConverter::InitializerListConverter(Cppyy::TCppType_t klass, std::string const &value_type) + + : InstanceConverter{klass}, + fValueTypeName{value_type}, + fValueType{Cppyy::GetScope(value_type)}, + fValueSize{Cppyy::SizeOf(value_type)} +{ +} + CPyCppyy::InitializerListConverter::~InitializerListConverter() { - if (fConverter && fConverter->HasState()) delete fConverter; + for (Converter *converter : fConverters) { + if (converter && converter->HasState()) delete converter; + } if (fBuffer) Clear(); } @@ -2948,7 +2959,8 @@ bool CPyCppyy::InitializerListConverter::SetArg( PyObject* item = PySequence_GetItem(pyobject, i); bool convert_ok = false; if (item) { - if (!fConverter) { + Converter *converter = CreateConverter(fValueTypeName); + if (!converter) { if (CPPInstance_Check(item)) { // by convention, use byte copy memcpy((char*)fake->_M_array + i*fValueSize, @@ -2968,7 +2980,10 @@ bool CPyCppyy::InitializerListConverter::SetArg( "default ctor needed for initializer list of objects"); } } - if (memloc) convert_ok = fConverter->ToMemory(item, memloc); + if (memloc) { + convert_ok = converter->ToMemory(item, memloc); + } + fConverters.emplace_back(converter); } @@ -3122,8 +3137,7 @@ CPyCppyy::Converter* CPyCppyy::CreateConverter(const std::string& fullType, cdim // get the type of the list and create a converter (TODO: get hold of value_type?) auto pos = realType.find('<'); std::string value_type = realType.substr(pos+1, realType.size()-pos-2); - return new InitializerListConverter(Cppyy::GetScope(realType), - CreateConverter(value_type), Cppyy::GetScope(value_type), Cppyy::SizeOf(value_type)); + return new InitializerListConverter(Cppyy::GetScope(realType), value_type); } //-- still nothing? use a generalized converter diff --git a/src/DeclareConverters.h b/src/DeclareConverters.h index 6d8acaee..a0985a7b 100644 --- a/src/DeclareConverters.h +++ b/src/DeclareConverters.h @@ -449,9 +449,7 @@ class SmartPtrConverter : public Converter { // initializer lists class InitializerListConverter : public InstanceConverter { public: - InitializerListConverter(Cppyy::TCppType_t klass, - Converter* cnv, Cppyy::TCppType_t valuetype, size_t sz) : InstanceConverter(klass), - fBuffer(nullptr), fConverter(cnv), fValueType(valuetype), fValueSize(sz) {} + InitializerListConverter(Cppyy::TCppType_t klass, std::string const& value_type); InitializerListConverter(const InitializerListConverter&) = delete; InitializerListConverter& operator=(const InitializerListConverter&) = delete; virtual ~InitializerListConverter(); @@ -464,8 +462,9 @@ class InitializerListConverter : public InstanceConverter { void Clear(); protected: - void* fBuffer; - Converter* fConverter; + void* fBuffer = nullptr; + std::vector fConverters; + std::string fValueTypeName; Cppyy::TCppType_t fValueType; size_t fValueSize; };