Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Re: I think b630937 (SvUOK docs) is wrong

10 views
Skip to first unread message

bulk88

unread,
Dec 24, 2012, 9:15:32 PM12/24/12
to Father Chrysostomos, perl5-...@perl.org
Father Chrysostomos wrote:
> I think b630937 (SvUOK docs) is wrong
>
> I find this commit questionable:
>
> From b630937b8bf49e835d8976fc1036e68c79585b04 Mon Sep 17 00:00:00 2001
> From: Karl Williamson <pub...@khwilliamson.com>
> Date: Mon, 24 Dec 2012 08:39:58 -0700
> Subject: [PATCH] perlapi: Fix misstatement
>
> According to the comments for Perl_sv_setuv(), for performance reasons,
> a UV that fits in an IV is stored as an IV.
> ---
> sv.h | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/sv.h b/sv.h
> index b841464..f01a91c 100644
> --- a/sv.h
> +++ b/sv.h
> @@ -655,10 +655,12 @@ Tells an SV that it is an integer and disables all other OK bits.
> Tells an SV that it is an unsigned integer and disables all other OK bits.
>
> =for apidoc Am|bool|SvIOK_UV|SV* sv
> -Returns a boolean indicating whether the SV contains an unsigned integer.
> +Returns a boolean indicating whether the SV contains an unsigned integer
> +that is too large to store as an IV.
>
> =for apidoc Am|bool|SvUOK|SV* sv
> -Returns a boolean indicating whether the SV contains an unsigned integer.
> +Returns a boolean indicating whether the SV contains an unsigned integer
> +that is too large to store as an IV.
>
> =for apidoc Am|bool|SvIOK_notUV|SV* sv
> Returns a boolean indicating whether the SV contains a signed integer.

The "for performance reasons" might be removed in the future if someone
does research and sees its not beneficial for some reason or another. It
might also be changed for CPU-specific reasons. That detail should not
be codified. Non core XS code might not follow that rule (a UV in range
of an IV is stored as an IV) when creating a UV without sv_setuv.

Karl Williamson

unread,
Dec 24, 2012, 11:55:14 PM12/24/12
to bulk88, Father Chrysostomos, perl5-...@perl.org
I don't understand most of what you are saying. I did this because I
got burned. In blead, doing a SvUOK(newSVuv(1)) returns FALSE. That
needs to be either changed or documented. Investigation showed that it
would return TRUE iff the value being stored won't fit in an IV. I
hoped that documenting it might cause people to question the current design.

Patches or wording suggestions welcome.

bulk88

unread,
Dec 25, 2012, 12:26:44 AM12/25/12
to Karl Williamson, Father Chrysostomos, perl5-...@perl.org
Karl Williamson wrote:

>
> I don't understand most of what you are saying. I did this because I
> got burned. In blead, doing a SvUOK(newSVuv(1)) returns FALSE. That
> needs to be either changed or documented. Investigation showed that it
> would return TRUE iff the value being stored won't fit in an IV. I
> hoped that documenting it might cause people to question the current
> design.
>
> Patches or wording suggestions welcome.

My wording suggestion.

___________________________________________________________
Returns a boolean indicating whether the SV contains an integer which
must be interpreted as unsigned. An integer whose value is within the
range of both an IV and an UV maybe be flagged as either.
___________________________________________________________

Another note for nitpicking, none of the *OK macros return booleans (1
or 0). They return 0 or not 0. I don't want to say true/false either
since that means something else in C++. Not sure if its worth correcting
the "boolean" word or not, or its obvious that you will learn very quick
to not compare it to 1.

Karl Williamson

unread,
Jan 13, 2013, 10:11:38 PM1/13/13
to bulk88, Father Chrysostomos, perl5-...@perl.org
On 12/24/2012 10:26 PM, bulk88 wrote:
> Karl Williamson wrote:
>
>>
>> I don't understand most of what you are saying. I did this because I
>> got burned. In blead, doing a SvUOK(newSVuv(1)) returns FALSE. That
>> needs to be either changed or documented. Investigation showed that
>> it would return TRUE iff the value being stored won't fit in an IV. I
>> hoped that documenting it might cause people to question the current
>> design.
>>
>> Patches or wording suggestions welcome.
>
> My wording suggestion.
>
> ___________________________________________________________
> Returns a boolean indicating whether the SV contains an integer which
> must be interpreted as unsigned. An integer whose value is within the
> range of both an IV and an UV maybe be flagged as either.

What would the implications be of changing the macro to match the prior
documentation, and return true if it is indeed SvIOK with a positive number?

> ___________________________________________________________
>
> Another note for nitpicking, none of the *OK macros return booleans (1
> or 0). They return 0 or not 0. I don't want to say true/false either
> since that means something else in C++. Not sure if its worth correcting
> the "boolean" word or not, or its obvious that you will learn very quick
> to not compare it to 1.
>

I looked at the code, and the macros I saw returned the result of a
logical expression, which according to K&R is 0 or 1. Does your
compiler not follow that?

bulk88

unread,
Jan 14, 2013, 1:15:20 AM1/14/13
to Karl Williamson, Father Chrysostomos, perl5-...@perl.org
Karl Williamson wrote:
> On 12/24/2012 10:26 PM, bulk88 wrote:
>> Karl Williamson wrote:
>>
>>>
>>> I don't understand most of what you are saying. I did this because I
>>> got burned. In blead, doing a SvUOK(newSVuv(1)) returns FALSE. That
>>> needs to be either changed or documented. Investigation showed that
>>> it would return TRUE iff the value being stored won't fit in an IV. I
>>> hoped that documenting it might cause people to question the current
>>> design.
>>>
>>> Patches or wording suggestions welcome.
>>
>> My wording suggestion.
>>
>> ___________________________________________________________
>> Returns a boolean indicating whether the SV contains an integer which
>> must be interpreted as unsigned. An integer whose value is within the
>> range of both an IV and an UV maybe be flagged as either.
>
> What would the implications be of changing the macro to match the prior
> documentation, and return true if it is indeed SvIOK with a positive
> number?

Longer machine code to put a >/< in there. Currently it is

_____________________________________________________________________
#define SvIOK_UV(sv) ((SvFLAGS(sv) & (SVf_IOK|SVf_IVisUV)) \
== (SVf_IOK|SVf_IVisUV))
#define SvUOK(sv) SvIOK_UV(sv)
_____________________________________________________________________

The other thing is, the flag checkers represent flags. Flags don't
guarantee behavior or a statement of fact outright. Just because an SV
is SVt_PVMG does not mean it is magical. I *guess* just because a SV has
SVs_GMG doesn't mean that xmg_u.xmg_magic is not NULL (not sure if such
a SV is following the rules or not).

I'll quote
http://www.nntp.perl.org/group/perl.perl5.porters/2012/02/msg183615.html

"Hence I don't think that "expanding"
SvOK_off() is the way to go, as it's *supposed* to be flag manipulation
macro, not a function with side effects.
"

Adding a gt/lt to SvUOK would mean it stops being purely flag manipulation.

XS code wise/back compat, IDK the implications. I would look at whether
non 2s compliment platforms might have bugs come up (are there any? does
Perl guarantee compatibility with non twos complement C compilers?).
Someone (I think its unlikely, I am not going to grep cpan right now)
might be using the UV flag on IV range integers as bit stealing to
encode something.

>
>> ___________________________________________________________
>>
>> Another note for nitpicking, none of the *OK macros return booleans (1
>> or 0). They return 0 or not 0. I don't want to say true/false either
>> since that means something else in C++. Not sure if its worth correcting
>> the "boolean" word or not, or its obvious that you will learn very quick
>> to not compare it to 1.
>>
>
> I looked at the code, and the macros I saw returned the result of a
> logical expression, which according to K&R is 0 or 1. Does your
> compiler not follow that?

My mistake, most (not all) of the *OK macros don't return booleans.
___________________________________________
SvNIOK(svSvNIOK);
SvIOK(svSvIOK);
SvIsUV(svSvIsUV);
___________________________________________
becomes
___________________________________________
((svSvNIOK)->sv_flags & (0x00000100|0x00000200));
((svSvIOK)->sv_flags & 0x00000100);
((svSvIsUV)->sv_flags & 0x80000000);
___________________________________________

I wrote that sentence generically.

Dave Mitchell

unread,
Jan 14, 2013, 11:45:56 AM1/14/13
to bulk88, Karl Williamson, Father Chrysostomos, perl5-...@perl.org
On Mon, Jan 14, 2013 at 01:15:20AM -0500, bulk88 wrote:
> >What would the implications be of changing the macro to match the
> >prior documentation, and return true if it is indeed SvIOK with a
> >positive number?
>
> Longer machine code to put a >/< in there. Currently it is
>
> _____________________________________________________________________
> #define SvIOK_UV(sv) ((SvFLAGS(sv) & (SVf_IOK|SVf_IVisUV)) \
> == (SVf_IOK|SVf_IVisUV))
> #define SvUOK(sv) SvIOK_UV(sv)
> _____________________________________________________________________

Normally we'd use cBOOL, which is defined (for maximum portability)
as

#define cBOOL(cbool) ((cbool) ? (bool)1 : (bool)0)

> The other thing is, the flag checkers represent flags. Flags don't
> guarantee behavior or a statement of fact outright.

It depends.

> Just because an
> SV is SVt_PVMG does not mean it is magical.

No, because that's not a flag, its a container type (which is capable of
supporting, but may or may not, have magic).

> I *guess* just because a
> SV has SVs_GMG doesn't mean that xmg_u.xmg_magic is not NULL (not
> sure if such a SV is following the rules or not).

I would regard an SV with SVs_GMG but without xmg_u.xmg_magic as a bug.
S_sv_unmagicext_flags() explicitly clears the magic bits if there's no
magic left.

>
> I'll quote http://www.nntp.perl.org/group/perl.perl5.porters/2012/02/msg183615.html
>
> "Hence I don't think that "expanding"
> SvOK_off() is the way to go, as it's *supposed* to be flag manipulation
> macro, not a function with side effects.
> "
>
> Adding a gt/lt to SvUOK would mean it stops being purely flag manipulation.

Its not a flag *manipulation* macro, its a flag *testing* macro,
documented to return a boolean value indicating whether a particular state
(usually a combination of flag bits, but sometimes other things too) is
true. So returning a boolean value is the right thing to do, modulo
performance and back-compat concerns.

I'd suggest waiting till after 5.18 is out, then change them and see what
breaks. (That gives us a whole year to find out).

It would also be good if someone could compare the assembly produced by say

if(SvUOK) { ... }

and

if(cBOOL(SvUOK)) { ... }

and see if the second is less efficient.

--
Never work with children, animals, or actors.

bulk88

unread,
Jan 14, 2013, 6:28:15 PM1/14/13
to Dave Mitchell, Karl Williamson, Father Chrysostomos, perl5-...@perl.org
Dave Mitchell wrote:
>
> It would also be good if someone could compare the assembly produced by say
>
> if(SvUOK) { ... }
>
> and
>
> if(cBOOL(SvUOK)) { ... }
>
> and see if the second is less efficient.
>
______________________________________________________________


void
SvUOKtest(sv)
SV * sv
PPCODE:
if(SvUOK(sv)) {
printf("I am SvUOK\n");
}
else {
printf("I am not SvUOK\n");
}

void
cBOOLSvUOKtest(sv)
SV * sv
PPCODE:
if(cBOOL(SvUOK(sv))) {
printf("I am cBOOLSvUOKtest\n");
}
else {
printf("I am not cBOOLSvUOKtest\n");
}
______________________________________________________________
3484: XS_EUPXS(XS_Local__XS_SvUOKtest); /* prototype to pass
-Wmissing-prototypes */
3485: XS_EUPXS(XS_Local__XS_SvUOKtest)
3486: {
003C5E5B 53 push ebx
003C5E5C 56 push esi
3487: dVAR; dXSARGS;
003C5E5D 8B 74 24 0C mov esi,dword ptr [esp+0Ch]
003C5E61 8B 46 48 mov eax,dword ptr [esi+48h]
003C5E64 8B 08 mov ecx,dword ptr [eax]
003C5E66 8B 56 0C mov edx,dword ptr [esi+0Ch]
003C5E69 83 C0 FC add eax,0FFFFFFFCh
003C5E6C 89 46 48 mov dword ptr [esi+48h],eax
003C5E6F 57 push edi
003C5E70 8B 3E mov edi,dword ptr [esi]
003C5E72 8B D9 mov ebx,ecx
003C5E74 C1 E3 02 shl ebx,2
003C5E77 8B C7 mov eax,edi
003C5E79 2B C3 sub eax,ebx
003C5E7B 2B C2 sub eax,edx
003C5E7D C1 F8 02 sar eax,2
3488: if (items != 1)
003C5E80 83 F8 01 cmp eax,1
003C5E83 74 0F je $L118730 (3C5E94h)
3489: croak_xs_usage(cv, "sv");
003C5E85 68 A4 8A 3C 00 push offset string "sv" (3C8AA4h)
003C5E8A FF 74 24 18 push dword ptr [esp+18h]
003C5E8E FF 15 08 71 3C 00 call dword ptr
[__imp__Perl_croak_xs_usage (3C7108h)]
3490: PERL_UNUSED_VAR(ax); /* -Wall */
3491: SP -= items;
3492: {
3493: SV * sv = ST(0)
3494: ;
3495: if(SvUOK(sv)) {
003C5E94 8B 44 8A 04 mov eax,dword ptr [edx+ecx*4+4]
003C5E98 8B 48 08 mov ecx,dword ptr [eax+8]
003C5E9B B8 00 01 00 80 mov eax,80000100h
003C5EA0 23 C8 and ecx,eax
003C5EA2 83 EF 04 sub edi,4
003C5EA5 3B C8 cmp ecx,eax
003C5EA7 75 07 jne $L118730+1Ch (3C5EB0h)
3496: printf("I am SvUOK\n");
003C5EA9 68 CC 80 3C 00 push offset string "I am SvUOK\n"
(3C80CCh)
3497: }
3498: else {
003C5EAE EB 05 jmp $L118730+21h (3C5EB5h)
3499: printf("I am not SvUOK\n");
003C5EB0 68 BC 80 3C 00 push offset string "I am not SvUOK\n"
(3C80BCh)
003C5EB5 FF 15 B4 70 3C 00 call dword ptr [__imp__printf (3C70B4h)]
003C5EBB 59 pop ecx
3500: }
3501: PUTBACK;
003C5EBC 89 3E mov dword ptr [esi],edi
003C5EBE 5F pop edi
003C5EBF 5E pop esi
003C5EC0 5B pop ebx
3502: return;
3503: }
3504: }
003C5EC1 C3 ret
3505:
3506:
3507: XS_EUPXS(XS_Local__XS_cBOOLSvUOKtest); /* prototype to pass
-Wmissing-prototypes */
3508: XS_EUPXS(XS_Local__XS_cBOOLSvUOKtest)
3509: {
003C5EC2 53 push ebx
003C5EC3 56 push esi
3510: dVAR; dXSARGS;
003C5EC4 8B 74 24 0C mov esi,dword ptr [esp+0Ch]
003C5EC8 8B 46 48 mov eax,dword ptr [esi+48h]
003C5ECB 8B 08 mov ecx,dword ptr [eax]
003C5ECD 8B 56 0C mov edx,dword ptr [esi+0Ch]
003C5ED0 83 C0 FC add eax,0FFFFFFFCh
003C5ED3 89 46 48 mov dword ptr [esi+48h],eax
003C5ED6 57 push edi
003C5ED7 8B 3E mov edi,dword ptr [esi]
003C5ED9 8B D9 mov ebx,ecx
003C5EDB C1 E3 02 shl ebx,2
003C5EDE 8B C7 mov eax,edi
003C5EE0 2B C3 sub eax,ebx
003C5EE2 2B C2 sub eax,edx
003C5EE4 C1 F8 02 sar eax,2
3511: if (items != 1)
003C5EE7 83 F8 01 cmp eax,1
003C5EEA 74 0F je $L118724 (3C5EFBh)
3512: croak_xs_usage(cv, "sv");
003C5EEC 68 A4 8A 3C 00 push offset string "sv" (3C8AA4h)
003C5EF1 FF 74 24 18 push dword ptr [esp+18h]
003C5EF5 FF 15 08 71 3C 00 call dword ptr
[__imp__Perl_croak_xs_usage (3C7108h)]
3513: PERL_UNUSED_VAR(ax); /* -Wall */
3514: SP -= items;
3515: {
3516: SV * sv = ST(0)
3517: ;
3518: if(cBOOL(SvUOK(sv))) {
003C5EFB 8B 44 8A 04 mov eax,dword ptr [edx+ecx*4+4]
003C5EFF 8B 48 08 mov ecx,dword ptr [eax+8]
003C5F02 B8 00 01 00 80 mov eax,80000100h
003C5F07 23 C8 and ecx,eax
003C5F09 83 EF 04 sub edi,4
003C5F0C 3B C8 cmp ecx,eax
003C5F0E 75 07 jne $L118724+1Ch (3C5F17h)
3519: printf("I am cBOOLSvUOKtest\n");
003C5F10 68 A4 80 3C 00 push offset string "I am
cBOOLSvUOKtest\n" (3C80A4h)
3520: }
3521: else {
003C5F15 EB 05 jmp $L118724+21h (3C5F1Ch)
3522: printf("I am not cBOOLSvUOKtest\n");
003C5F17 68 88 80 3C 00 push offset string "I am not
cBOOLSvUOKtest\n" (3C8088h)
003C5F1C FF 15 B4 70 3C 00 call dword ptr [__imp__printf (3C70B4h)]
003C5F22 59 pop ecx
3523: }
3524: PUTBACK;
003C5F23 89 3E mov dword ptr [esi],edi
003C5F25 5F pop edi
003C5F26 5E pop esi
003C5F27 5B pop ebx
3525: return;
3526: }
3527: }
003C5F28 C3 ret
________________________________________________________________________

It is the same. Compiled on 32 bit VC 2003 with " 'OPTIMIZE' => '
-O1 -G7 -GL -Oi -Og -arch::SSE2 '".


davem, do you have an opinion on khw's
> What would the implications be of changing the macro to match the prior
> documentation, and return true if it is indeed SvIOK with a positive
> number?

which I understand to mean adding lt/gt check against the UV/IV in SvUOK
macro?

Dave Mitchell

unread,
Jan 18, 2013, 6:56:02 AM1/18/13
to bulk88, Karl Williamson, Father Chrysostomos, perl5-...@perl.org
On Mon, Jan 14, 2013 at 06:28:15PM -0500, bulk88 wrote:
> davem, do you have an opinion on khw's
> > What would the implications be of changing the macro to match the prior
> > documentation, and return true if it is indeed SvIOK with a positive
> > number?
>
> which I understand to mean adding lt/gt check against the UV/IV in
> SvUOK macro?

I don't like this idea. I'm happy with your suggested doc change, which
documents the status quo:

> Returns a boolean indicating whether the SV contains an integer which
> must be interpreted as unsigned. An integer whose value is within the
> range of both an IV and an UV maybe be flagged as either.

--
The warp engines start playing up a bit, but seem to sort themselves out
after a while without any intervention from boy genius Wesley Crusher.
-- Things That Never Happen in "Star Trek" #17

Karl Williamson

unread,
Jan 23, 2013, 2:00:06 PM1/23/13
to Dave Mitchell, bulk88, Father Chrysostomos, perl5-...@perl.org
On 01/18/2013 04:56 AM, Dave Mitchell wrote:
> On Mon, Jan 14, 2013 at 06:28:15PM -0500, bulk88 wrote:
>> davem, do you have an opinion on khw's
>>> What would the implications be of changing the macro to match the prior
>>> documentation, and return true if it is indeed SvIOK with a positive
>>> number?
>>
>> which I understand to mean adding lt/gt check against the UV/IV in
>> SvUOK macro?
>
> I don't like this idea. I'm happy with your suggested doc change, which
> documents the status quo:
>
>> Returns a boolean indicating whether the SV contains an integer which
>> must be interpreted as unsigned. An integer whose value is within the
>> range of both an IV and an UV maybe be flagged as either.
>

Now changed with commit a6ceea0637411cc48e4e043c7d222d707dd3611a

I changed Darin's wording slightly so as to not imply that a negative
number could be SvUOK.

Darin McBride

unread,
May 22, 2013, 10:47:57 AM5/22/13
to perl5-...@perl.org
Just for kicks, I was going through the git log and found this.

I didn't suggest the wording, bulk88 did :) But, thanks for the credit anyway
:D
signature.asc
0 new messages