fix sv_numeq and add other SV numeric comparison APIs #23966
Conversation
|
Draft for now, I suspect the overloading support has problems when the current op context is void. |
tonycoz
force-pushed
the
23918-cmp-api
branch
from
403367e to
13ed11e
Compare
some refactoring next, since sv_numeq_flags and sv_numne_flags are similar. Used a separate test file since putting every sv_num*() variant in the one file would be ugly Addresses GH Perl#23918 but isn't a direct fix
If nothing else putting them together may avoid someone doing `!sv_numeq(...)`
do_ncmp() expects simple SVs and for overloaded SVs will just compare the SvNV() of each SV, mishandling the case where the 0+ overload returns a large UV or IV that isn't exactly representable as an NV. # Conflicts: # ext/XS-APItest/t/sv_numeq.t # ext/XS-APItest/t/sv_numne.t # sv.c
These are all needed because overloading may make them inconsistent with <=> overloading.
similar to try_amagic_bin()
Discovered while working on another module, in many amagic_call() will use the current context when calling the overload sub, but these APIs might be called in XS code that simply needs a comparison, regardless of the current OP context.
and use it from the numeric comparison APIs.
instead of a special case in amagic_call()
modified the sv.c documentation since the perldelta sv_numeq link had multiple targets.
tonycoz
force-pushed
the
23918-cmp-api
branch
from
13ed11e to
096a2ec
Compare
|
There was some discussion in IRC on how to handle overloading. In particular, amagic_call() will check if the I can see these and similar APIs being called in a few contexts:
I think the default (sv_numeq(), the non-flags APIs) should overload based on the current state, which is the current behaviour Setting Setting I plan to add a flags variant of sv_2num() (which isn't API /cry) and a flag for amagic_call() to force overload and ignore |
leonerd
left a comment
leonerd
left a comment
There was a problem hiding this comment.
Overall this seems a good direction. A couple of small comments.
|
|
||
| #define AMGf_want_list 0x0040 | ||
| #define AMGf_numarg 0x0080 | ||
| #define AMGf_force_scalar 0x0100 |
There was a problem hiding this comment.
This style is fine, but I think in general I usually add these in the form (1<<0), (1<<1), etc to make it clearer they are single bitflags, and easier to see gaps or where to add the next.
| * regardless of the overall context of the multiconcat op | ||
| */ | ||
| U8 gimme = (force_scalar || !PL_op || PL_op->op_type == OP_MULTICONCAT) | ||
| U8 gimme = (force_scalar || (flags & AMGf_force_scalar) || !PL_op ) |
There was a problem hiding this comment.
++
I definitely like that this decouples the function at least from deep knowledge of being OP_MULTICONCAT.
| } | ||
|
|
||
| PERL_STATIC_INLINE bool | ||
| S_sv_numcmp_common(pTHX_ SV **sv1, SV **sv2, const U32 flags, |
There was a problem hiding this comment.
Took me a moment to realise this function isn't doing the regular non-overloaded comparison. It might do with adding a comment, saying something like:
this function only handles overloaded comparison, returning true if the comparison is already done. if it returns false, then the caller should compare numerically using do_ncmp().
| { | ||
| $Config{d_double_has_nan} | ||
| or skip "No NAN", 2; | ||
| my $nan = 0+"NaN"; |
There was a problem hiding this comment.
We have builtin::nan now ;)
(repeated variously throughout the test files)
| # GMAGIC | ||
| "10" =~ m/(\d+)/; | ||
| is sv_numcmp_flags($1, 10, 0), -1, 'sv_numcmp_flags with no flags does not GETMAGIC'; | ||
| is sv_numcmp_flags($1, 10, SV_GMAGIC), 0, 'sv_numecmp_flags with SV_GMAGIC does'; |
2 participants
As discussed in #23918 this adds APIs corresponding to the other perl numeric comparison operators.
Separate APIs for each comparison a provided as each calls the appropriate overload for that comparison.
This also fixes a bug in sv_numeq() which didn't correctly handle the case where the operator itself isn't overloaded, but conversion to a number is, and that conversion returns a large integer not representable as a NV.