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.

Sign up for GitHub

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

Jump to bottom

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

Closed
guitargeek opened this issue Feb 12, 2024 · 5 comments · Fixed by #14
Closed

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

guitargeek opened this issue Feb 12, 2024 · 5 comments · Fixed by #14

Comments

@guitargeek
Copy link
Contributor

guitargeek commented Feb 12, 2024 •
edited
Loading

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:
root-project/root#14507
@wlav
Copy link
Owner

wlav commented Feb 18, 2024

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

wlav commented Feb 18, 2024

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 added a commit to guitargeek/CPyCppyy that referenced this issue Feb 19, 2024
@guitargeek
Carry a copy of the string also in the std::string_view converter
a5222b7 
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.
@guitargeek guitargeek mentioned this issue Feb 19, 2024
Merged
@guitargeek
Copy link
Contributor Author

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 added a commit to guitargeek/CPyCppyy that referenced this issue Feb 19, 2024
@guitargeek
Carry a copy of the string also in the std::string_view converter
137b29e 
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.
guitargeek added a commit to guitargeek/CPyCppyy that referenced this issue Feb 19, 2024
@guitargeek
Carry a copy of the string also in the std::string_view converter
975b3c0 
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.
guitargeek added a commit to guitargeek/CPyCppyy that referenced this issue Feb 19, 2024
@guitargeek
Carry a copy of the string also in the std::string_view converter
d913614 
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.
guitargeek added a commit to guitargeek/CPyCppyy that referenced this issue Feb 19, 2024
@guitargeek
Carry a copy of the string also in the std::string_view converter
e3011c3 
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.
@wlav wlav closed this as completed in #14 Mar 8, 2024
@guitargeek
Copy link
Contributor Author

The reproducer regressed with fce87d5.

guitargeek added a commit to guitargeek/cppyy that referenced this issue Apr 9, 2024
@guitargeek
Add unit test for function that takes multiple string views
b0174d6 
Taken from wlav/CPyCppyy#13
@guitargeek guitargeek mentioned this issue Apr 9, 2024
Open
guitargeek added a commit to guitargeek/cppyy that referenced this issue Apr 9, 2024
@guitargeek
Add unit test for function that takes multiple string views
0ecbad8 
Taken from wlav/CPyCppyy#13
@wlav
Copy link
Owner

wlav commented Apr 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@wlav @guitargeek