wlav / CPyCppyy

Other
23 stars 20 forks source link

Wrong passing of arguments when function takes multiple args of type `string_view` #13

Closed guitargeek closed 6 months ago

guitargeek commented 7 months ago

Reproducer:

import cppyy

cppyy.cppdef("""
void foo(std::string_view s1, std::string_view s2)
{
   std::cout << s1 << std::endl;
   std::cout << s2 << std::endl;
}
""")

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

Output:

world
world

I opened this in the CPyCppyy repo, because I first observed this problem when trying to use the new CPyCppyy in PyROOT: https://github.com/root-project/root/pull/14507

wlav commented 7 months ago

The problem is here: https://github.com/wlav/CPyCppyy/blob/master/src/Converters.cxx#L1821

Contrary to the other string classes, using an std::string_view as a buffer, doesn't buffer the data. The code as written expects the data from the converted bytes object to be copied into the buffer. The solution would be to add the bytes object as a temporary to the call context to be cleaned up after the call, or to add a life line to it (same).

wlav commented 7 months ago

Note that the simpler option of adding an std::string data member to contain the buffered data would incur a copy that setting a lifeline would not. That may sound wasteful, but copying that string will be way faster for small strings than setting a lifeline. I don't know where the cut-off is, but I suspect that the extra copy would not be unreasonable in practice.

guitargeek commented 7 months ago

Thank you very much for your comments! I have opened a PR that suggests the copying fix. I will also do the change in the ROOT PR, so it can be tested.

guitargeek commented 5 months ago

The reproducer regressed with https://github.com/wlav/CPyCppyy/commit/fce87d5e0125bb9e84ea3472dae6643faa5b8aed.

wlav commented 5 months ago

Fixed again; different method: doesn't use a buffer in the converter.