Add function amagic_find(sv, method, flags) #20604
Conversation
|
@demerphq : @book mentioned that this function would be useful to some of your work independent of #20503 . In this patch, the function is currently stand-alone and without tests; do you have thoughts about how it should be tested in isolation? There are perhaps opportunities to reduce some duplication, as there are similar uses in Perl_amagic_call, however to make that sort of refactoring readable, I think we'd want to introduce a struct to hold the pointers to the mg, stash, cvp, etc. ... this patch does not attempt that. |
ericherman
force-pushed
the
ericherman/amagic_find
branch
3 times, most recently
from
aced732 to
319e4f8
Compare
ericherman
changed the title
ericherman
force-pushed
the
ericherman/amagic_find
branch
3 times, most recently
from
2c89e29 to
27e2960
Compare
|
On Sun, 11 Dec 2022, 11:42 Eric Herman, ***@***.***> wrote:
@demerphq <https://github.com/demerphq> : @book <https://github.com/book>
mentioned that this function would be useful to some of your work
independent of #20503 <#20503> . In
this patch, the function is currently stand-alone and without tests; do you
have thoughts about how it should be tested in isolation?
Basically it should agree with overload::Method. Usually you test this sort
of thing in ext/XSAPI-test (however it is spelled) by creating an XS
function to wrap the function you want to test, and then add a perl .t test
file to call it. For instance, if I have an object with certain overloads
the wrapper and o::Method should agree in a boolean context about whether
an object implements an overload method.
FWIW, The XSAPI-test xs file is huge, you can add new sections to create
new test modules and new tests fairly easily. Check it's changelog for
examples.
For instance, I'd expect a simple wrapper that takes an sv and an integer,
and returns a bool whether that sv is a reference with that integer
overload method enabled.
You then cross check that with overload::Method, you just need to account
for it using a string for the method, and the internals needs an integer
constant. I would just bake the constant into the test. Eg, array
drreferencing is '@{}' in overload::Method.
There are perhaps opportunities to reduce some duplication, as there are
similar uses in Perl_amagic_call, however to make that sort of refactoring
readable, I think we'd want to introduce a struct to hold the pointers to
the mg, stash, cvp, etc.
I am not so much thinking about that level. I was thinking about maybe
rewriting overload.pm to use it.
and but this patch does not attempt that.
Sure, makes sense. Thanks for this, I'm looking forward to making use of it.
Yves
… |
|
A thought on testing: Would it be possible to adjust the code in |
As I mentioned, there is an opportunity to do a bigger refactoring, however the callers stash a lot of intermediate pointers (mg, stash, cvp, etc), thus it is not a straight-forward refactoring. We could change amagic_find to receive pointers-to-pointers as parameters, or introduce a parameter struct to hold the pointers, however it is not clear to me that either of these would be a great improvement. I think it is possible, yes, however I do not have a good vision of what a true improvement of extracting the duplication of amagic_call would look like. |
Returns the CV pointer to the overloaded method, which will be needed by join to detect concat magic. Co-authored-by: Philippe Bruhat (BooK) <book@cpan.org>
ericherman
force-pushed
the
ericherman/amagic_find
branch
from
27e2960 to
37a1830
Compare
|
@ericherman is there any reason this is still marked draft? Id be fine with applying it as is. |
+1. It looks fine to me. |
|
Once its merged we can add tests after. Probably faster. :-) |
|
Only that we didn't have tests in place yet. However, I'm happy to see it tested indirectly, as will happen with RFC0013 and your work @demerphq |
4 participants
Returns the CV pointer to the overloaded method, which will be needed by join to detect concat magic.
Co-authored-by: @book