HV_FETCH_ISSTORE and G_DISCARD have the same value

[mauke]

Description

I don't have time to investigate this more closely at the moment, but I don't want to lose track of it either; hence this issue.

perldoc perlapi says the hash deletion functions (hv_delete, hv_deletes, hv_delete_ent) can take a flags value of G_DISCARD to make them return NULL (instead of the value that was deleted from the hash).

Internally, hv_delete(hv, key, klen, flags) forwards to hv_common_key_len(hv, key, klen, flags | HV_DELETE, NULL, 0), which in turn calls hv_common(hv, NULL, key, abs(klen), klen < 0 ? HVhek_UTF8 : 0, flags, NULL, 0), which is a macro wrapper for Perl_hv_common. Perl_hv_common then takes the flags argument (confusingly named action in this function) to determine what to do, comparing bit tests against HV_DELETE, HV_FETCH_ISSTORE, HV_FETCH_ISEXISTS, etc.

These constants are defined in hv.h next to a comment that says their values must not overlap with G_DISCARD:

#define HV_DISABLE_UVAR_XKEY	0x01
/* We need to ensure that these don't clash with G_DISCARD, which is 2, as it
   is documented as being passed to hv_delete().  */
#define HV_FETCH_ISSTORE	0x04
#define HV_FETCH_ISEXISTS	0x08

This makes sense because callers can just bitwise-or with G_DISCARD and we don't want Perl_hv_common to get confused about what was passed.

However, G_DISCARD is not 2; it is 4 and so overlaps with HV_FETCH_ISSTORE:

#define G_DISCARD         0x4	/* Call FREETMPS.
                                   Don't change this without consulting the
                                   hash actions codes defined in hv.h */

The comments in cop.h and hv.h were added in commit b54b483 on 2007-09-19, and then shortly after disregarded in commit 2f8edad on 2008-01-21, when G_DISCARD was changed from 2 to 4 without fixing up the constants in hv.h.

Steps to Reproduce

I haven't yet figured out what, if anything, this breaks. :-(

Expected behavior

The comments should match the code.

[bulk88]

hv_common()'s prototype is a disaster.

void * Perl_hv_common(#1 interpreter *my_perl, #2 HV *hv, #3 SV *keysv, #4 char *key, #5 STRLEN klen, #6 int flags, #7 int action, #8 SV *val, #9 U32 hash)

There are no optimizations any CC can do about the code above, Linux only allows 6 C args before degrading to C stack transport. I see 9 arguments. If only XS developers had to fill out all 9 arguments each and every time they want to call it, it would be fixed fast.

```Perl_hv_commonhas 2 very wasteful, especially since 64b CPU became cheap, argumentsint flags` and `int action` Both are 32 bit types promoted to 64 bits, wasting a minimum of 1 `void *` register. `int flags` seems to only exist to transport `UTF8` info since

#define HVhek_UTF8 0x01 #define HVhek_WASUTF8 0x02 #define HVhek_NOTSHARED 0x04 #define HVhek_FREEKEY 0x100
collide with
#define HV_DISABLE_UVAR_XKEY 0x01 #define HV_FETCH_ISSTORE 0x04 #define HV_FETCH_ISEXISTS 0x08 #define HV_FETCH_LVALUE 0x10 #define HV_FETCH_JUST_SV 0x20 #define HV_DELETE 0x40 #define HV_FETCH_EMPTY_HE 0x80 in lowest U8 #1 of 4xU8=U32.

I some how see but can't explain your G_DISCARD 0x02 colliding with /* Must not conflict with HVhek_UTF8 */ #define HV_NAME_SETALL 0x02 but HV_NAME_SETALL seems to be exclusive to Perl_hv_undef_flags, but, but...

but would someone ever want a nuke-entire-HV*-container-but-transfer-RC-ownership-of-its-insides-to-me-feature?

In Perl 5 that feature is called @arr = %{$hashref}; or @arr = %{{k1 => 'v1, k2 =>'v2'}}; or another_sub(somesub($self, {k1 => 'v1', k2 =>'v2'})). Plus = qq|hello camels ${\join('',"not a bobby drop tables exploit", $obj->find('thing'))}|;

The string G_DISCARD being used anywhere outside of the OP* API is a poor style IMO. HV* API's integer 0x02 constant should be renamed to something starting with the letters HV_ and legacy G_DISCARD gets shadow banned.

Since the point of FOSS to to make SW for people other than yourself and mine or yours intelligence level, it always needs to be thought about how an API's spelling/design/common steps looks like to part timers.

Ponder this, if the docs say I can use constant G_DISCARD with the hv_*() functions, why can't I use G_VOID with the hv_*() functions?

Maybe I had no sleep and save the token G_VOID (BAD) in the file where G_DISCARD (GOOD) was supposed to go?

They sound identical unless I grep the .pod.

TBH, if I was short on time, never touched Perl's C code before, and only had 10 mins of SO/PMonks/AI psuedoinfo in my head about Perl in C , I would make a guessG_DISCARD sounds like one of Larrys joke names , and the some legal document (Ex. https://developers.google.com/style/word-list ) mandated the identifier G_DISCARD be changed to G_VOID, and that they are the same integer under the hood, which in real life code, that mistake could be a zero side effects NOOP at runtime, or a major rapid OOM/SEGV time leak.

I'm not a fan of #define SV_CATUTF8 0x00008000 and #define SVf_UTF8 0x20000000 being randomly used in func params of the same API family.

#define G_DISCARD 0x00000002 is the synonym of #define SVs_TEMP 0x00080000, hv_*()/av_*() want G_DISCARD 0x00000002 but newSVpvn_flags wants SVs_TEMP 0x00080000, and call_sv() wants G_DISCARD+G_VOID (I think, not sure if auto correct logic exists).

I'm not sure what the fix if any would loo like.

A bunch of other constants that I see as duplicates in my head.

#define SV_GMAGIC (1 << 1) /* 0x0002 - 2 */ collides with #define G_SCALAR and SV_GMAGIC is a dupe or competition to #define GV_AUTOLOAD 0x100 #define GV_CROAK 0x200 #define GV_ADDMG 0x400 /* add if magical */ #define GV_NO_SVGMAGIC 0x800 /* Skip get-magic on an SV argument used only by gv_fetchsv(_nomg) */ and #define HV_DISABLE_UVAR_XKEY 0x01 and #define SV_SKIP_OVERLOAD (1 << 13) /* 0x2000 - 8192 */ are the same.

-Invoke user code or not?

#define HV_FETCH_ISSTORE 0x04 and #define HV_FETCH_LVALUE 0x10 and #define GV_ADD 0x01 and
#define GV_ADDMULTI 0x02 are the same.

Getters that can not fail (they vivify)

#define HVhek_FREEKEY 0x100 /* Internal flag to say key is Newx()ed. */ and the inverse of #define SV_NOSTEAL (1 << 4) /* 0x0010 - 16 */ and #define SV_IMMEDIATE_UNREF (1 << 0) /* 0x0001 - 1 */ and #define G_DISCARD 0x4 are the same concept,

"Destroy/release as much as possible, I don't want to do manual GC/RC tracking."