Skip to content

Commit

Permalink
Carry a copy of the string also in the std::string_view converter
Browse files Browse the repository at this point in the history
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<std::string_view> sl)
{
   for (auto const& s : sl) {
      std::cout << s << std::endl;
   }
   std::cout << std::endl;
}
""")

cppyy.gbl.foo("hello", "world")
```

Closes wlav#13.
  • Loading branch information
guitargeek committed Feb 19, 2024
1 parent 6539c24 commit a5222b7
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 11 deletions.
26 changes: 18 additions & 8 deletions src/Converters.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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; \
} \
Expand Down Expand Up @@ -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<std::string*>(para.fValue.fVoidp);
para.fValue.fVoidp = &fStringView;
return true;
}

if (!CPPInstance_Check(pyobject))
return false;
Expand All @@ -1884,8 +1890,12 @@ 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;
}
Expand All @@ -1902,9 +1912,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;
}
Expand Down
10 changes: 7 additions & 3 deletions src/DeclareConverters.h
Original file line number Diff line number Diff line change
Expand Up @@ -346,18 +346,20 @@ class VoidPtrPtrConverter : public Converter {

CPPYY_DECLARE_BASIC_CONVERTER(PyObject);


// A stateful string converter that always buffers a copy of the string.
// This buffering is important also for the string_view case, otherwise the
// pointed-to string might not live long enough. See also:
// https://github.com/wlav/CPyCppyy/issues/13
#define CPPYY_DECLARE_STRING_CONVERTER(name, strtype) \
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; \
std::basic_string<strtype::value_type> fStringBuffer; \
}

CPPYY_DECLARE_STRING_CONVERTER(STLString, std::string);
Expand All @@ -366,6 +368,8 @@ CPPYY_DECLARE_STRING_CONVERTER(STLStringViewBase, std::string_view);
class STLStringViewConverter : public STLStringViewBaseConverter {
public:
virtual bool SetArg(PyObject*, Parameter&, CallContext* = nullptr);
private:
std::string_view fStringView;
};
#endif
CPPYY_DECLARE_STRING_CONVERTER(STLWString, std::wstring);
Expand Down

0 comments on commit a5222b7

Please sign in to comment.