Skip to content

Optimize some use of SvUOK() to SvIsUV() #23914

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: blead
Choose a base branch
from
+17 −17
Open

Optimize some use of SvUOK() to SvIsUV() #23914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac5e48f
sv.h: Make SvIsUV() return 1 (rather than 0x80000000) for true condition
t-a-k May 7, 2025
9a330bd
pp.c, pp_hot.c: Use SvIsUV rather than SvUOK after SvIV_please_nomg
t-a-k May 7, 2025
0b5d78a
hv.c: Use SvIsUV instead of SvUOK in the block where SvIOK holds true.
t-a-k May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3856,7 +3856,7 @@ Perl_refcounted_he_new_pvn(pTHX_ struct refcounted_he *parent,
} else if (SvPOK(value)) {
value_type = HVrhek_PV;
} else if (SvIOK(value)) {
value_type = SvUOK((const SV *)value) ? HVrhek_UV : HVrhek_IV;
value_type = SvIsUV(value) ? HVrhek_UV : HVrhek_IV;
} else if (!SvOK(value)) {
value_type = HVrhek_undef;
} else {
Expand Down
26 changes: 13 additions & 13 deletions pp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ PP(pp_pow)
bool baseuok;
UV baseuv;

if (SvUOK(svr)) {
if (SvIsUV(svr)) {
power = SvUVX(svr);
} else {
const IV iv = SvIVX(svr);
Expand All @@ -1165,7 +1165,7 @@ PP(pp_pow)
}
}

baseuok = SvUOK(svl);
baseuok = SvIsUV(svl);
if (baseuok) {
baseuv = SvUVX(svl);
} else {
Expand Down Expand Up @@ -1375,8 +1375,8 @@ PP(pp_multiply)
we know the left is integer. */
/* Left operand is defined, so is it IV? */
if (SvIV_please_nomg(svl)) {
bool auvok = SvUOK(svl);
bool buvok = SvUOK(svr);
bool auvok = SvIsUV(svl);
bool buvok = SvIsUV(svr);
UV alow;
UV blow;
UV product;
Expand Down Expand Up @@ -1469,8 +1469,8 @@ PP(pp_divide)

#ifdef PERL_TRY_UV_DIVIDE
if (SvIV_please_nomg(svr) && SvIV_please_nomg(svl)) {
bool left_non_neg = SvUOK(svl);
bool right_non_neg = SvUOK(svr);
bool left_non_neg = SvIsUV(svl);
bool right_non_neg = SvIsUV(svr);
UV left;
UV right;

Expand Down Expand Up @@ -1588,7 +1588,7 @@ PP(pp_modulo)
SV * const svr = PL_stack_sp[0];
SV * const svl = PL_stack_sp[-1];
if (SvIV_please_nomg(svr)) {
right_neg = !SvUOK(svr);
right_neg = !SvIsUV(svr);
if (!right_neg) {
right = SvUVX(svr);
} else {
Expand Down Expand Up @@ -1618,7 +1618,7 @@ PP(pp_modulo)
a UV. In range NV has been rounded down to nearest UV and
use_double false. */
if (!use_double && SvIV_please_nomg(svl)) {
left_neg = !SvUOK(svl);
left_neg = !SvIsUV(svl);
if (!left_neg) {
left = SvUVX(svl);
} else {
Expand Down Expand Up @@ -1911,7 +1911,7 @@ PP(pp_subtract)
} else {
/* Left operand is defined, so is it IV? */
if (SvIV_please_nomg(svl)) {
if ((auvok = SvUOK(svl)))
if ((auvok = SvIsUV(svl)))
auv = SvUVX(svl);
else {
const IV aiv = SvIVX(svl);
Expand All @@ -1929,7 +1929,7 @@ PP(pp_subtract)
bool result_good = 0;
UV result;
UV buv;
bool buvok = SvUOK(svr);
bool buvok = SvIsUV(svr); /* svr is always IOK here */

if (buvok)
buv = SvUVX(svr);
Expand Down Expand Up @@ -2248,9 +2248,9 @@ Perl_do_ncmp(pTHX_ SV* const left, SV * const right)
#ifdef PERL_PRESERVE_IVUV
/* Fortunately it seems NaN isn't IOK */
if (SvIV_please_nomg(right) && SvIV_please_nomg(left)) {
if (!SvUOK(left)) {
if (!SvIsUV(left)) {
const IV leftiv = SvIVX(left);
if (!SvUOK(right)) {
if (!SvIsUV(right)) {
/* ## IV <=> IV ## */
const IV rightiv = SvIVX(right);
return (leftiv > rightiv) - (leftiv < rightiv);
Expand All @@ -2265,7 +2265,7 @@ Perl_do_ncmp(pTHX_ SV* const left, SV * const right)
}
}

if (SvUOK(right)) {
if (SvIsUV(right)) {
/* ## UV <=> UV ## */
const UV leftuv = SvUVX(left);
const UV rightuv = SvUVX(right);
Expand Down
4 changes: 2 additions & 2 deletions pp_hot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1920,7 +1920,7 @@ PP(pp_add)
} else {
/* Left operand is defined, so is it IV? */
if (SvIV_please_nomg(svl)) {
if ((auvok = SvUOK(svl)))
if ((auvok = SvIsUV(svl)))
auv = SvUVX(svl);
else {
const IV aiv = SvIVX(svl);
Expand All @@ -1938,7 +1938,7 @@ PP(pp_add)
bool result_good = 0;
UV result;
UV buv;
bool buvok = SvUOK(svr);
bool buvok = SvIsUV(svr); /* svr is always IOK here */

if (buvok)
buv = SvUVX(svr);
Expand Down
2 changes: 1 addition & 1 deletion sv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,7 +1045,7 @@ Set the size of the string buffer for the SV. See C<L</SvLEN>>.
#define BOOL_INTERNALS_sv_isbool_false(sv) (SvIsCOW_static(sv) && \
(SvPVX_const(sv) == PL_No))

#define SvIsUV(sv) (SvFLAGS(sv) & SVf_IVisUV)
#define SvIsUV(sv) ((SvFLAGS(sv) & SVf_IVisUV) != 0)
Copy link
Contributor

@richardleach richardleach Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem necessary - surely the truthiness is already being returned?

Copy link
Contributor Author

@t-a-k t-a-k Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this change is technically not necessary in C99, but I think this will help keeping the code less surprising.

SvFLAGS(sv) & SVf_IVisUV takes 0 or 0x80000000 and assigning this result into bool value (which is not guaranteed to be as wide as 0x80000000) is not intuitive I think (although this is not real problem in C99).

#define SvIsUV_on(sv) (SvFLAGS(sv) |= SVf_IVisUV)
#define SvIsUV_off(sv) (SvFLAGS(sv) &= ~SVf_IVisUV)

Expand Down
Loading