wlav / cppyy

Other
412 stars 42 forks source link

Add unit test for function that takes multiple string views #228

Open guitargeek opened 7 months ago

guitargeek commented 7 months ago

Taken from https://github.com/wlav/CPyCppyy/issues/13

wlav commented 7 months ago

The test needs a doc string (see other examples) as that will be printed when running pytest -v.

guitargeek commented 7 months ago

Ok! I'm still trying to understand why the tests fails with CPyCppyy master.

If I remove this newly added code here, it works: https://github.com/wlav/CPyCppyy/blob/master/src/Converters.cxx#L1950

// for Python str object: convert to single char string in buffer and take a view
    if (CPyCppyy_PyUnicodeAsBytes2Buffer(pyobject, fStringBuffer)) {
        fStringViewBuffer = fStringBuffer;
        para.fValue.fVoidp = &fStringViewBuffer;
        para.fTypeCode = 'V';
        return true;
    }

But I can't see anything wrong with it!

HasState() of the STLStringViewConverter is correctly overridden...

guitargeek commented 7 months ago

Hi @wlav, I understand now better what is going on, see this simpler reproducer:

import cppyy

s1 = cppyy.gbl.std.string(b"s1")
s2 = cppyy.gbl.std.string(b"s2")

v1 = cppyy.gbl.std.string_view(s1)
print(v1)

cppyy.gbl.std.string_view(s2)
print(v1)
s1
s2

The problem is not the string converter in particular, but the converter logic in general.

Even though the converters proclaim that they have state with HasState(), cppyy doesn't respect that over multiple function calls. The string converter used for the first std::string_view is exactly the same object as for the second string view.

Do you have any implementation ideas to fix this? I'll open a separate issue to continue the discussion.

wlav commented 7 months ago

Issue is fixed in repo, but probably useful to add further tests.