Skip to content

[PyROOT] Fix std::vector<char> iteration#9708

Merged
etejedor merged 2 commits intoroot-project:masterfrom
etejedor:vectoriter
Jan 31, 2022
Merged

[PyROOT] Fix std::vector<char> iteration#9708
etejedor merged 2 commits intoroot-project:masterfrom
etejedor:vectoriter

Conversation

@etejedor
Copy link
Copy Markdown
Contributor

Fixes #9632 (see the text of the first commit for more details).

As part of the implementation of std.vector.__iter__, the vector's
data() method is called. In the case of a vector<char>, data()
returns a char*, which is converted by cppyy to a Python string.
The issue is that the conversion char*->str expects that the
sequence of characters to convert is null-terminated, e.g. in
Python3 the following function is used for the conversion:
https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_FromString

In the case of the data pointed by the char* returned by data(),
there is no guarantee that the sequence of characters will be
null-terminated, and this results in unpredictable and erroneous
behaviour.

The fix of this commit adds a pythonization for vector<char>::data()
that temporarily adds a null character to the vector before calling
the actual data(), so that the conversion is done correctly and
the returned Python string contains the characters of the vector.
This also fixes the vector<char> iteration, which relies on data().
@etejedor etejedor requested a review from eguiraud January 26, 2022 11:33
@etejedor etejedor self-assigned this Jan 26, 2022
@etejedor etejedor requested a review from bellenot as a code owner January 26, 2022 11:33
@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

Copy link
Copy Markdown
Contributor

@eguiraud eguiraud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried this might have unexpected consequences on performance and behavior, but after discussion with Enric and due to my lack of better ideas... :)

@etejedor
Copy link
Copy Markdown
Contributor Author

To leave here some details of the discussion: the use of vector<char>::data() happens during the creation of the iterator, not during the iteration itself, so the performance impact is much smaller. As for the change in behaviour, the pythonization reverts the temporary addition immediately after the call to the real data(), so the changes are never seen by the user.

@etejedor etejedor merged commit c89362d into root-project:master Jan 31, 2022
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.

Iteration on std.vector['char'] is broken

3 participants