From 243b574bcb0da431b283f5109582ba70f9d244d2 Mon Sep 17 00:00:00 2001 From: James Raspass Date: Tue, 20 Aug 2024 05:56:59 +0100 Subject: [PATCH 1/3] Remove "perl_" prefix from "perl_require_pv" As per the docs, the perl_require_pv form is deprecated. Note I think the second of these calls is actually a bug since profile_class is a package name with colons, not a filename with slashes. We're not currently checking ERRSV after this call so it's probably silently failing. But such a fix is out of scope of this commit. Also it's probably worth moving to load_module instead as per the docs: It is analogous to the Perl code eval "require '$file'". It's even implemented that way; consider using load_module instead. --- DBI.xs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/DBI.xs b/DBI.xs index 9b4dd216..97aac296 100644 --- a/DBI.xs +++ b/DBI.xs @@ -2151,7 +2151,7 @@ dbih_set_attr_k(SV *h, SV *keysv, int dbikey, SV *valuesv) dSP; I32 returns; TAINT_NOT; /* the require is presumed innocent till proven guilty */ - perl_require_pv("DBI/Profile.pm"); + require_pv("DBI/Profile.pm"); if (SvTRUE(ERRSV)) { warn("Can't load %s: %s", profile_class, SvPV_nolen(ERRSV)); valuesv = &PL_sv_undef; @@ -2173,7 +2173,7 @@ dbih_set_attr_k(SV *h, SV *keysv, int dbikey, SV *valuesv) if (on && !sv_isobject(valuesv)) { /* not blessed already - so default to DBI::Profile */ HV *stash; - perl_require_pv(profile_class); + require_pv(profile_class); stash = gv_stashpv(profile_class, GV_ADDWARN); sv_bless(valuesv, stash); } From 58127fa72ac400e7e65cbbf9b390da199ba03cc7 Mon Sep 17 00:00:00 2001 From: James Raspass Date: Tue, 20 Aug 2024 06:04:05 +0100 Subject: [PATCH 2/3] Use mPUSH macros to replace sv_2mortal Also use an XPUSH macro to replace as explicit EXTEND. --- DBI.xs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/DBI.xs b/DBI.xs index 97aac296..6c39164e 100644 --- a/DBI.xs +++ b/DBI.xs @@ -771,7 +771,7 @@ set_err_sv(SV *h, imp_xxh_t *imp_xxh, SV *err, SV *errstr, SV *state, SV *method neatsvpv(method,0) ); PUSHMARK(SP); - XPUSHs(sv_2mortal(newRV_inc((SV*)DBIc_MY_H(imp_xxh)))); + mXPUSHs(newRV_inc((SV*)DBIc_MY_H(imp_xxh))); XPUSHs(err); XPUSHs(errstr); XPUSHs(state); @@ -2158,7 +2158,7 @@ dbih_set_attr_k(SV *h, SV *keysv, int dbikey, SV *valuesv) } else { PUSHMARK(SP); - XPUSHs(sv_2mortal(newSVpv(profile_class,0))); + mXPUSHs(newSVpv(profile_class, 0)); XPUSHs(valuesv); PUTBACK; returns = call_method("_auto_new", G_SCALAR); @@ -2938,7 +2938,7 @@ dbi_profile(SV *h, imp_xxh_t *imp_xxh, SV *statement_sv, SV *method, NV t1, NV t EXTEND(SP, 4); PUSHMARK(SP); PUSHs(h); /* push inner handle, then others params */ - PUSHs( sv_2mortal(newSVpv(method_pv,0))); + mPUSHs(newSVpv(method_pv, 0)); PUTBACK; SAVE_DEFSV; /* local($_) = $statement */ DEFSV_set(statement_sv); @@ -3719,7 +3719,7 @@ XS(XS_DBI_dispatch) imp_msv = (SV*)gv_fetchmethod_autoload(DBIc_IMP_STASH(imp_xxh), "func", FALSE); if (imp_msv) { /* driver does have func method so undo the earlier 'func' stack changes */ - PUSHs(sv_2mortal(newSVpv(meth_name,0))); + mPUSHs(newSVpv(meth_name, 0)); PUTBACK; ++items; meth_name = "func"; @@ -3925,7 +3925,7 @@ XS(XS_DBI_dispatch) /* and may mess up the error handling below for the commit/rollback */ PUSHMARK(SP); XPUSHs(h); - XPUSHs(sv_2mortal(newSVpv("AutoCommit",0))); + mXPUSHs(newSVpv("AutoCommit", 0)); XPUSHs(&PL_sv_yes); PUTBACK; call_method("STORE", G_VOID); @@ -4082,7 +4082,7 @@ XS(XS_DBI_dispatch) ); PUSHMARK(SP); XPUSHs(msg); - XPUSHs(sv_2mortal(newRV_inc((SV*)DBIc_MY_H(imp_xxh)))); + mXPUSHs(newRV_inc((SV*)DBIc_MY_H(imp_xxh))); XPUSHs( result ); PUTBACK; items = call_sv(*hook_svp, G_SCALAR); @@ -5576,8 +5576,7 @@ set_err(h, err, errstr=&PL_sv_no, state=&PL_sv_undef, method=&PL_sv_undef, resul } else (void)SvOK_off(*sem_svp); - EXTEND(SP, 1); - PUSHs( result ? result : &PL_sv_undef ); + XPUSHs( result ? result : &PL_sv_undef ); } /* We don't check RaiseError and call die here because that must be */ /* done by returning through dispatch and letting the DBI handle it */ From ef97f356c37506e098c6aa6571c9be824ac53b54 Mon Sep 17 00:00:00 2001 From: James Raspass Date: Tue, 20 Aug 2024 06:08:28 +0100 Subject: [PATCH 3/3] Use -s macros with literals to avoid magic numbers Note hv_stores also drops the trailing hash param from its hv_store counterpart as the hash is computed automatically at compile time. --- DBI.xs | 56 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/DBI.xs b/DBI.xs index 6c39164e..331eff13 100644 --- a/DBI.xs +++ b/DBI.xs @@ -200,7 +200,7 @@ typedef struct dbi_ima_st { (SvOK(state) /* SQLSTATE is implemented by driver */ \ ? (strEQ(SvPV_nolen(state),"00000") ? &PL_sv_no : sv_mortalcopy(state))\ : (SvTRUE(DBIc_ERR(imp_xxh)) \ - ? sv_2mortal(newSVpv("S1000",5)) /* General error */ \ + ? sv_2mortal(newSVpvs("S1000")) /* General error */ \ : &PL_sv_no) /* Success ("00000") */ \ ) @@ -425,7 +425,7 @@ _join_hash_sorted(HV *hash, char *kv_sep, STRLEN kv_sep_len, char *pair_sep, STR keys = _sort_hash_keys(hash, num_sort, &total_len); if (!keys) - return newSVpv("", 0); + return newSVpvs(""); if (!kv_sep_len) kv_sep_len = strlen(kv_sep); @@ -446,7 +446,7 @@ _join_hash_sorted(HV *hash, char *kv_sep, STRLEN kv_sep_len, char *pair_sep, STR if (!hash_svp) { /* should never happen */ warn("No hash entry with key '%s'", keys[i]); - sv_catpvn(return_sv, "???", 3); + sv_catpvs(return_sv, "???"); continue; } @@ -457,11 +457,11 @@ _join_hash_sorted(HV *hash, char *kv_sep, STRLEN kv_sep_len, char *pair_sep, STR if (SvOK(*hash_svp)) { STRLEN hv_val_len; char *hv_val = SvPV(*hash_svp, hv_val_len); - sv_catpvn(return_sv, "'", 1); + sv_catpvs(return_sv, "'"); sv_catpvn(return_sv, hv_val, hv_val_len); - sv_catpvn(return_sv, "'", 1); + sv_catpvs(return_sv, "'"); } - else sv_catpvn(return_sv, "undef", 5); + else sv_catpvs(return_sv, "undef"); } if (i < hv_len-1) @@ -612,14 +612,14 @@ neatsvpv(SV *sv, STRLEN maxlen) /* return a tidy ascii value, for debugging only if (SvMAGICAL(sv)) { if (DBIS_TRACE_LEVEL >= 5) { /* add magic details to help debugging */ MAGIC* mg; - infosv = sv_2mortal(newSVpv(" (magic-",0)); - if (SvSMAGICAL(sv)) sv_catpvn(infosv,"s",1); - if (SvGMAGICAL(sv)) sv_catpvn(infosv,"g",1); - if (SvRMAGICAL(sv)) sv_catpvn(infosv,"r",1); - sv_catpvn(infosv,":",1); + infosv = sv_2mortal(newSVpvs(" (magic-")); + if (SvSMAGICAL(sv)) sv_catpvs(infosv, "s"); + if (SvGMAGICAL(sv)) sv_catpvs(infosv, "g"); + if (SvRMAGICAL(sv)) sv_catpvs(infosv, "r"); + sv_catpvs(infosv, ":"); for (mg = SvMAGIC(sv); mg; mg = mg->mg_moremagic) sv_catpvn(infosv, &mg->mg_type, 1); - sv_catpvn(infosv, ")", 1); + sv_catpvs(infosv, ")"); } if (SvGMAGICAL(sv) && !PL_dirty) mg_get(sv); /* trigger magic to FETCH the value */ @@ -689,7 +689,7 @@ neatsvpv(SV *sv, STRLEN maxlen) /* return a tidy ascii value, for debugging only SvGROW(nsv, (1+maxlen+1+1)); sv_setpvn(nsv, quote, 1); sv_catpvn(nsv, v, maxlen-3); /* account for three dots */ - sv_catpvn(nsv, "...", 3); + sv_catpvs(nsv, "..."); } else { SvGROW(nsv, (1+len+1+1)); sv_setpvn(nsv, quote, 1); @@ -720,7 +720,7 @@ copy_statement_to_parent(pTHX_ SV *h, imp_xxh_t *imp_xxh) if (parent && SvROK(parent)) { SV *tmp_sv = *hv_fetchs((HV*)SvRV(h), "Statement", 1); if (SvOK(tmp_sv)) - (void)hv_store((HV*)SvRV(parent), "Statement", 9, SvREFCNT_inc(tmp_sv), 0); + (void)hv_stores((HV*)SvRV(parent), "Statement", SvREFCNT_inc(tmp_sv)); } } @@ -813,7 +813,7 @@ set_err_sv(SV *h, imp_xxh_t *imp_xxh, SV *err, SV *errstr, SV *state, SV *method if (SvTRUE(h_state) && SvTRUE(state) && strNE(SvPV_nolen(h_state), SvPV_nolen(state))) sv_catpvf(h_errstr, " [state was %s now %s]", SvPV_nolen(h_state), SvPV_nolen(state)); if (strNE(SvPV_nolen(h_errstr), SvPV_nolen(errstr))) { - sv_catpvn(h_errstr, "\n", 1); + sv_catpvs(h_errstr, "\n"); sv_catsv(h_errstr, errstr); } } @@ -1484,7 +1484,7 @@ dbih_setup_handle(pTHX_ SV *orv, char *imp_class, SV *parent, SV *imp_datasv) && SvROK(*tmp_svp) && SvTYPE(SvRV(*tmp_svp)) == SVt_PVHV ) { /* XXX mirrors behaviour of dbih_set_attr_k() of Callbacks */ - (void)hv_store((HV*)SvRV(h), "Callbacks", 9, newRV_inc(SvRV(*tmp_svp)), 0); + (void)hv_stores((HV*)SvRV(h), "Callbacks", newRV_inc(SvRV(*tmp_svp))); DBIc_set(imp, DBIcf_Callbacks, 1); } @@ -1522,16 +1522,16 @@ dbih_setup_handle(pTHX_ SV *orv, char *imp_class, SV *parent, SV *imp_datasv) switch (DBIc_TYPE(imp)) { case DBIt_DB: /* cache _inner_ handle, but also see quick_FETCH */ - (void)hv_store((HV*)SvRV(h), "Driver", 6, newRV_inc(SvRV(parent)), 0); + (void)hv_stores((HV*)SvRV(h), "Driver", newRV_inc(SvRV(parent))); (void)hv_fetchs((HV*)SvRV(h), "Statement", 1); /* store writable undef */ break; case DBIt_ST: DBIc_NUM_FIELDS((imp_sth_t*)imp) = -1; /* cache _inner_ handle, but also see quick_FETCH */ - (void)hv_store((HV*)SvRV(h), "Database", 8, newRV_inc(SvRV(parent)), 0); + (void)hv_stores((HV*)SvRV(h), "Database", newRV_inc(SvRV(parent))); /* copy (alias) Statement from the sth up into the dbh */ tmp_svp = hv_fetchs((HV*)SvRV(h), "Statement", 1); - (void)hv_store((HV*)SvRV(parent), "Statement", 9, SvREFCNT_inc(*tmp_svp), 0); + (void)hv_stores((HV*)SvRV(parent), "Statement", SvREFCNT_inc(*tmp_svp)); break; } } @@ -1586,7 +1586,7 @@ static int dbih_dumpcom(pTHX_ imp_xxh_t *imp_xxh, const char *msg, int level) { dMY_CXT; - SV *flags = sv_2mortal(newSVpv("",0)); + SV *flags = sv_2mortal(newSVpvs("")); SV *inner; static const char pad[] = " "; if (!msg) @@ -1834,7 +1834,7 @@ dbih_get_fbav(imp_sth_t *imp_sth) "0", 0, "Number of row fields inconsistent with NUM_OF_FIELDS (driver bug)", "", "_get_fbav"); /* DBIc_NUM_FIELDS(imp_sth) = i; - hv_delete((HV*)SvRV(sth), "NUM_OF_FIELDS", 13, G_DISCARD); + hv_deletes((HV*)SvRV(sth), "NUM_OF_FIELDS", G_DISCARD); */ } /* don't let SvUTF8 flag persist from one row to the next */ @@ -2752,14 +2752,14 @@ log_where(SV *buf, int append, char *prefix, char *suffix, int show_line, int sh dTHX; dTHR; if (!buf) - buf = sv_2mortal(newSVpv("",0)); + buf = sv_2mortal(newSVpvs("")); else if (!append) sv_setpv(buf,""); if (CopLINE(PL_curcop)) { COP *cop; dbi_caller_string(buf, PL_curcop, prefix, show_line, show_path); if (show_caller && (cop = dbi_caller_cop())) { - SV *via = sv_2mortal(newSVpv("",0)); + SV *via = sv_2mortal(newSVpvs("")); dbi_caller_string(via, cop, prefix, show_line, show_path); sv_catpvf(buf, " via %s", SvPV_nolen(via)); } @@ -2983,7 +2983,7 @@ dbi_profile(SV *h, imp_xxh_t *imp_xxh, SV *statement_sv, SV *method, NV t1, NV t else if (isGV(method)) { /* just using SvPV_nolen(method) sometimes causes an error: */ /* "Can't coerce GLOB to string" so we use gv_efullname() */ - SV *tmpsv = sv_2mortal(newSVpv("",0)); + SV *tmpsv = sv_2mortal(newSVpvs("")); gv_efullname4(tmpsv, (GV*)method, "", TRUE); p = SvPV_nolen(tmpsv); if (*p == '*') ++p; /* skip past leading '*' glob sigil */ @@ -3415,7 +3415,7 @@ XS(XS_DBI_dispatch) keep_error = TRUE; if (ima_flags & IMA_CLEAR_STMT) { /* don't use SvOK_off: dbh's Statement may be ref to sth's */ - (void)hv_store((HV*)SvRV(h), "Statement", 9, &PL_sv_undef, 0); + (void)hv_stores((HV*)SvRV(h), "Statement", &PL_sv_undef); } if (ima_flags & IMA_CLEAR_CACHED_KIDS) clear_cached_kids(aTHX_ h, imp_xxh, meth_name, trace_flags); @@ -3925,7 +3925,7 @@ XS(XS_DBI_dispatch) /* and may mess up the error handling below for the commit/rollback */ PUSHMARK(SP); XPUSHs(h); - mXPUSHs(newSVpv("AutoCommit", 0)); + mXPUSHs(newSVpvs("AutoCommit")); XPUSHs(&PL_sv_yes); PUTBACK; call_method("STORE", G_VOID); @@ -4605,7 +4605,7 @@ _new_handle(class, parent, attr_ref, imp_datasv, imp_class) PERL_UNUSED_VAR(cv); } - (void)hv_store((HV*)SvRV(attr_ref), "ImplementorClass", 16, SvREFCNT_inc(imp_class), 0); + (void)hv_stores((HV*)SvRV(attr_ref), "ImplementorClass", SvREFCNT_inc(imp_class)); /* make attr into inner handle by blessing it into class */ sv_bless(attr_ref, class_stash); @@ -4711,7 +4711,7 @@ _install_method(dbi_class, meth_name, file, attribs=Nullsv) { dMY_CXT; /* install another method name/interface for the DBI dispatcher */ - SV *trace_msg = (DBIS_TRACE_LEVEL >= 10) ? sv_2mortal(newSVpv("",0)) : Nullsv; + SV *trace_msg = (DBIS_TRACE_LEVEL >= 10) ? sv_2mortal(newSVpvs("")) : Nullsv; CV *cv; SV **svp; dbi_ima_t *ima;