Skip to content

Conversation

@guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Feb 19, 2024

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).

Also, the InitializerListConverter is changed such that it creates separate converters for each element if the converters have state.

This PR fixes the following reproducer:

If they have state, a separate converter needs to be created for each
element in the initializer list.

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")
cppyy.gbl.bar(["hello", "world"])

Closes #13.

@guitargeek guitargeek force-pushed the string_view_patch branch 2 times, most recently from 137b29e to 975b3c0 Compare February 19, 2024 18:29
@guitargeek
Copy link
Contributor Author

guitargeek commented Feb 19, 2024

The same patch is also now part of my syncing PR in ROOT:
root-project/root#14507
If the current CI run over there fixes these excessive failures in the DataFrame tutorials, it means it worked.

edit: indeed, the RDataFrame tutorials pass now

@guitargeek
Copy link
Contributor Author

Also: what is the most appropriate place for a unit test? I guess somewhere here?
https://github.com/wlav/cppyy/tree/master/test

A class with a virtual destructor should also define copying and
assignment. In the case of the converter class, both should be
forbidden.

This would have helped me understanding the code if it would have been
done before.
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.
If they have state, a separate converter needs to be created for each
element in the initializer list.

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")
cppyy.gbl.bar(["hello", "world"])
```
@guitargeek guitargeek changed the title Carry a copy of the string also in the std::string_view converter Carry a copy of the string also in the std::string_view converter and consider statefulness in InitializerListConverter Feb 20, 2024
@wlav wlav merged commit 73cbd9b into wlav:master Mar 8, 2024
@guitargeek guitargeek deleted the string_view_patch branch March 9, 2024 00:42
@wlav
Copy link
Owner

wlav commented Apr 4, 2024

This one actually breaks two existing string_view tests, so in the future, it would be useful to run the standard cppyy tests and not just the ROOT ones, which have clearly far less coverage.

The problem is that in SetArg, the assumption is made that the pointed to voidp is an std::string*, but by changing the buffer type in the declaration to std::string while leaving the scope id to std::string_view in the implementation, it can be either std::string* or std::string_view* and no way out to tell which is which. When wrong, then crash.

wlav added a commit that referenced this pull request Apr 4, 2024
@guitargeek
Copy link
Contributor Author

Thank you very much for following up! Sorry, I didn't think of this case. It's more common to use std::string_view as argument types and not pass them around directly, that's why I didn't catch this

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.

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

2 participants