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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guitargeek
Copy link
Contributor

Taken from wlav/CPyCppyy#13

@wlav
Copy link
Owner

wlav commented Apr 9, 2024

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

@guitargeek
Copy link
Contributor Author

guitargeek commented Apr 9, 2024

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
Copy link
Contributor Author

guitargeek commented Apr 9, 2024

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
Copy link
Owner

wlav commented Apr 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants