[PyROOT][7501] Fix initialization of C++ data members via attribute s…#8036
Conversation
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. |
|
Build failed on ROOT-fedora30/cxx14. |
|
Build failed on ROOT-fedora31/noimt. |
|
Build failed on ROOT-performance-centos8-multicore/default. |
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on mac1014/python3. |
|
Build failed on mac11.0/cxx17. Warnings:
And 1453 more |
|
Build failed on ROOT-debian10-i386/cxx14. |
|
@phsft-bot build |
|
Starting build on |
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
Thanks for finding this 😄 This is my first review of cppyy code so it's mostly questions and doubts, feel free to merge as it is!
| auto res = PyObject_SetAttrString(holder, (char*)attr_name.str().c_str(), target); | ||
| return res != -1; |
There was a problem hiding this comment.
Reading at the the signature of PyObject_SetAttrString, I have the following two questions:
- The second argument accepts a
const char *, which is already the return type ofattr_name.str().c_str(), is the extra(char*)needed? - The return value of
PyObject_SetAttrStringshould be0on success and1on failure according to the documentation. If this is true and there are no other cases not reported in the docs, these lines could be substituted by
| auto res = PyObject_SetAttrString(holder, (char*)attr_name.str().c_str(), target); | |
| return res != -1; | |
| return PyObject_SetAttrString(holder, attr_name.str().c_str(), target) == 0 |
There was a problem hiding this comment.
Ok for 1, and I'll break the str().c_str() chain. 2 is fine either way 😄
There was a problem hiding this comment.
Ad 1.: the (char*) is for older pythons and still appears in older code throughout CPyCppyy, so is retained for consistency. Ad 2.: it's -1 on failure, not 1 and it is likewise by convention: -1 is the common failure report across the C-API, whereas 0 is not a common success report across same.
| // info/status | ||
| uint64_t fFlags; | ||
| Cppyy::TCppScope_t fCurScope; | ||
| PyObject* fPyContext; |
There was a problem hiding this comment.
I am thinking it would be nice to have somewhere a few more docs about why this is needed. Maybe referincing the linked issue and saying the context will be used to set a lifeline to help in keeping the temporary arrays alive?
Maybe, Here could be a good place?
There was a problem hiding this comment.
Ok I'll add some explanation!
|
Build failed on windows10/cxx14. Errors:
|
|
Build failure on Windows to be checked with @bellenot when he is back, not merging for the moment. |
etejedor
force-pushed
the
array-class-members
branch
from
9bfda2c to
bf64b00
Compare
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
…etting Based on: https://bitbucket.org/wlav/cpycppyy/commits/ad2fb338544d8baf034825a0bc2dfc46c182f925 https://bitbucket.org/wlav/cpycppyy/commits/6dd664b59641d68dca856bc39ea3f9def9c19d8a https://bitbucket.org/wlav/cpycppyy/commits/d39cb178c841e99935262204d487b76705c03e5b
etejedor
force-pushed
the
array-class-members
branch
from
bf64b00 to
c216afe
Compare
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
And 229 more |
|
@phsft-bot build just on windows10/cxx14 |
|
Starting build on |
|
Build failed on windows10/cxx14. |
|
@phsft-bot build just on windows10/cxx14 |
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
|
@phsft-bot build just on windows10/cxx14 |
|
Starting build on |
|
Build failed on windows10/cxx14. Failing tests: |
|
@phsft-bot build just on windows10/cxx14 |
|
Starting build on |
5 participants
…etting
Based on:
https://bitbucket.org/wlav/cpycppyy/commits/ad2fb338544d8baf034825a0bc2dfc46c182f925
https://bitbucket.org/wlav/cpycppyy/commits/6dd664b59641d68dca856bc39ea3f9def9c19d8a
https://bitbucket.org/wlav/cpycppyy/commits/d39cb178c841e99935262204d487b76705c03e5b
Fixes #7501