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.

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

Remove unused arg and method. #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vgvassilev
Copy link

@vgvassilev vgvassilev commented Mar 11, 2023

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

wlav commented Mar 11, 2023

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

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

wlav commented Mar 11, 2023

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

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

@vgvassilev
Copy link
Author

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

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

@wlav
Copy link
Owner

wlav commented Mar 12, 2023

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

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

wlav commented Mar 13, 2023

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.

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.

2 participants