search

wlav / CPyCppyy

Other
23 stars 20 forks source link

Remove unused arg and method. #6

Open vgvassilev opened 1 year ago

vgvassilev commented 1 year ago

cc: @sudo-panda, if that gets merged please backport it to our fork as it would simplify the implementation of some of the interfaces in InterOp.

PS: There were supposed to be 2 separate commits but I see one with both changes. Let me know if I should split.

wlav commented 1 year ago

Problem is that the older interface is still being used in PyPy. Of course, there it's the C-API, not the C++ one, so as long as the C version can be reimplemented in the reduced C++ API, it's okay.

vgvassilev commented 1 year ago

Problem is that the older interface is still being used in PyPy. Of course, there it's the C-API, not the C++ one, so as long as the C version can be reimplemented in the reduced C++ API, it's okay.

If you meant to suggest more work for this PR, please give the code blocks where I need to do the change. Is this a another repo?

wlav commented 1 year ago

The C-API is also in clingwrapper.cxx, and currently, it just forwards (after dealing with things such as std::string and other C++ types): https://github.com/wlav/cppyy-backend/blob/master/clingwrapper/src/clingwrapper.cxx#L2766

vgvassilev commented 1 year ago

I deleted GetMethodPrototype the c-api seems to use GetMethodSignature. Am I missing something?

vgvassilev commented 1 year ago

I deleted GetMethodPrototype the c-api seems to use GetMethodSignature. Am I missing something?

PS: ah, sorry, can I drop that interface?

wlav commented 1 year ago

Again, on your side, sure. As a pull request on this repo, no, b/c PyPy. AFAICT, the C API still uses it: https://github.com/wlav/cppyy-backend/blob/master/clingwrapper/src/clingwrapper.cxx#L2766

vgvassilev commented 1 year ago

Again, on your side, sure. As a pull request on this repo, no, b/c PyPy. AFAICT, the C API still uses it: https://github.com/wlav/cppyy-backend/blob/master/clingwrapper/src/clingwrapper.cxx#L2766

Then we should not, because we want our side to get merged here. The problem with GetMethodSignature vs GetMethodPrototype (or whatever their exact naming is) is mostly in the show formal args parameter. The method signature shows the arguments and the method prototype is the type of the function so no default arguments can be printed.

AFAICT, the implementation does not make such distinction and mostly means give me the thing with the default args. I was not aware of PyPy's use case so not sure what's used there. To be honest it is a bit complicated to develop in this multirepo environment.

wlav commented 1 year ago

The problem with GetMethodSignature vs GetMethodPrototype (or whatever their exact naming is) is mostly in the show formal args parameter.

For improvement, maybe start with going back to their uses. There are 2: 1) for documentation (i.e. filling out the __doc__ strings), and 2) for overload resolution. For documentation, there's a long-standing request to pick up the comments, too (basically what THtml does/did), and so if Cling has more to offer, it could become GetDocumentation(). For overload selection, that's been a long-standing discussion. :) It could be improved within the scope of the Numba work, i.e. __reflex__. After those two upgrades, the whole thing can go.