sv.h: SvPVx now equivalent to SvPV, et.al

[khwilliamson]

These macros suffixed with 'x' are guaranteed to evaluate their arguments just once. Prior to this commit, they used PL_Sv to accomplish that. But since 1ef9039 in 5.37, the macros they call only evaluate their arguments once, so the PL_Sv is superfluous.

• This set of changes does not require a perldelta entry.

[bulk88]

If this were a perldoc comment (seen by CPAN XS authors), I'd strongly vocalize a note mentioning that this remark can not be used outside of blead perl/core only code, b/c compat with old perls. Out of paranoia for future edits, I'd change a word or 2 in "/* The macros these now expand to no longer evaluate their arguments multiple times */" to somehow suggest this comment is NA for CPAN.

[richardleach]

this remark can not be used outside of blead perl/core only code, b/c compat with old perls.

@bulk88 - I'm unclear on why XS authors would have to be copying definitions out of sv.h? They surely should use the API and that hasn't changed. I could see that changing the definitions in ppport.h - if any of these are in there - would not be backwards compatible.

@khwilliamson - apart from the above concern, LTGM.

[bulk88]

this remark can not be used outside of blead perl/core only code, b/c compat with old perls.

@bulk88 - I'm unclear on why XS authors would have to be copying definitions out of sv.h? They surely should use the API and that hasn't changed. I could see that changing the definitions in ppport.h - if any of these are in there - would not be backwards compatible.

ppport.h doesnt have any public api documentation content AFAIK, I've never seen any content or ever used it as a reference for CPAN code. only ref info ppport.h provides is API/macro add/drop timeline history.

I did note above

If this were a perldoc comment (seen by CPAN XS authors), I'd strongly vocalize a note mentioning that this remark can not be used outside of blead perl/core only code, b/c compat with old perls.

this is a C comment that will not be seen by 90% of XS authors. There is a small risk of future doc edits, which are "improvements" elevating obfuscated/UB legalese to metacpan/perldocs visibility, Those doc edits will NEVER be done by the same person who named the fn/macro/token/var, Most likely a doc/comment edit, is the 1st C/XS/core commit ever by that person, who might be college aged or younger (GSOC/interns/etc, bkgnd high tech skills or beginner irrelevant), I'd rather duplicate crit info docs wise in multiple places in core vs a wheres waldo hunt with grep,

The original phrasing/wording DOES say "used to be multi eval" but it took me 3 passes of the sentence to realize "used to be multi eval" fact was included. 2-3 words can be moved/changed so that fact is understood on eyeball pass 1 not 3.

IDC, but adding a period at the end would be nice since rest of the comment is long and looks like a sentence. I wouldn't have written a PR reply just for the missing period.

[richardleach]

I'm not clear what the best wording would be in that case.
If technically accurate, something like the following?

/* The following macro expansions evaluate their arguments just once. In
 * earlier perl releases, PL_Sv had to be used as an intermediate in order
 * to prevent multiple evaluations. The expansion behaviour changed in 
 * commit 1ef9039bccbfe64f47f201b6cfb7d6d23e0b08a7. */

[bulk88]

/* The following macro expansions evaluate their arguments just once. In

• earlier perl releases, PL_Sv had to be used as an intermediate in order
• to prevent multiple evaluations. The expansion behaviour changed in
• commit 1ef9039. */

my proposed wording below. To keep it short by text length, I'm not adding the fact that these macros are 100% identical to certain other macros in ultra new perls. The end reader can put 1+1 together, phrase In earlier perl releases, these macros used to provides the hint that legacy/grandfather/history is involved in the particular API fn call's docs.

"anti-multi-eval" describes former/current/cpan/old-perl-safe purpose of these exact tokens. to use more efficient inline functions instead of macros and a global var provides enough engineering info for the curious cat to skip doing "trust but verify" archaeology with git blame/ML.

Changing public API is very alarming to some people with personality disorders. Certain Comm/Foss SDKs with public APIs, just do a big red "REMOVED" or a big red "EOL" stamp, on deprecated/EOL scheduled func calls. And not saying in src comments, HTML comments, git comments or public facing bug trackers, why the func got a future removal in ~1 to ~5 years.

• Was it priv esc security/exploit?
• performance?
• race conditions?
• unfixable bugs b/c the fix breaks existing code?-
• ABI/API/var names/capitalization/spelling/arg types and order/etc?
• This fn is now a slightly slower back compat shim b/c a Forklift replacement of internals happened?
• Day 1->Now, Official API docs, official sample usage, was buggy, only solution is PerlCritic blacklisting, since there is zero non-buggy production code in the world. The new fixed call will get a new token/identifier. Buggy old impl is untouched.
• Community survey/citizen outrage/pitchforks?
• BOFH reasserting his power [check reddit/HN/the register/slashdot for details]

Basically a project admin group should note to 3P End Users the following, DO ABSOLUTELY NOTHING with your prior private/CPAN XS code. DO NOT EDIT or grep+replace all your CPAN XS libs, either slowly or quickly. Changelogs like MetaClass::getRecords() was altered in Wave 45, are useless, what is the end user supposed to do?

My proposed wording.

The following macro expansions evaluate their arguments just once on all versions of Perl. In earlier perl releases, these macros used PL_Sv global var as an intermediate in order to prevent multiple evaluations. The implementation changed in commit 1ef9039bccbfe64f47f201b6cfb7d6d23e0b08a7 to use more efficient inline functions instead of macros and a global var for their implementation,

[tonycoz]

I think the now in the comment makes this clear enough.

The perlapi documentation makes the historical behaviour clear enough for x vs non-x names.

[bulk88]

I think the now in the comment makes this clear enough.

The perlapi documentation makes the historical behaviour clear enough for x vs non-x names.

I know by heart what the XvXVv() macros are, but only because I'm old. Nobody younger than me will "get it" solo/fast/quick. A Perl C newbie introduced post-2019, would be disoriented looking at that screen of src code plus +/-1 screen up down. Perl C has the term "mathom", maybe that should be thrown in the comment. The word "mathom" instantly identifies that cluster of macros as a "this is for CPAN XS only, do not use inside non-CPAN Interp core" and tells the reader in 0.25 seconds to "Look away! Nothing to see here! Keep scrolling!".

"Mathom" is shortest by text length way to convey the concept why that code is there and what/where its used vs mine/other ppls rewording so far.

Just like a doctor or pilot, all professionals only remember 80-90% off the top of their head. The last 10% is "open book" college exams. Pros are paid for their speed vs general public, not their smile or hair. Id rather see the code easier to read, vs constantly grepping the huge perlintern.pod and perlguts.pod. In my opinion reordering/moving 2-3 words or adding 2 or 3 words in the KHW original commit is enough to close this ticket.

I care more about future people looking at the code comment. Everyone who clicked on this ticket or knows about this Github account, already has the info in that comment memorized.

[bulk88]

I approve this commit for merging now, regardless of my minor criticism on the src code comments.

Since it does a very very small machine code reduction for all MSVC, to not write one ptr (after all -o1/liveness analysis) to my_perl->ISv per usage of that macro.

With this commit SV* gets cached/anti-multi-eval protection through the basic feature of using static inline funcs, which are real funcs, but disappear/optimize away at -o1 -o2., and with this commit the SV* arg is bouced around cheap and free volatile registers only. Its never stored for 1 nanosecond to the C stack or stored for 1 nanosecond to the my_perl struct ( malloc block).

SvPV(_sv, len) nowadays is a static inline, ancient history it was a multi evaling [hazard] macro. Dont want to git blame when that was done, but the fix to SvPV(_sv, len) was done a couple years ago.

[khwilliamson]

I used a combination of the suggested wordings