Skip to content

sv.c: make PUSH_EXTEND_MORTAL__SV_C an inline function #23234

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.

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 1 commit into
base: blead
Choose a base branch
from

Conversation

mauke
Copy link
Contributor

@mauke mauke commented May 1, 2025

This macro could easily be a function, so why not make it one?


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

This macro could easily be a function, so why not make it one?
@ap ap added the defer-next-dev This PR should not be merged yet, but await the next development cycle label May 1, 2025
@richardleach
Copy link
Contributor

When doing something like sv_2mortal(newSViv(27)), the checks in Perl_sv_2mortal for (!sv) and SvIMMORTAL(sv) are unnecessary and what remains is this new function's body plus SvTEMP_on(sv).

Is it worth making this function Perl_push_extend_mortal and part of the API, whether that's in sv.c or in sv_inline.h?

@bulk88
Copy link
Contributor

bulk88 commented May 9, 2025

Bad idea, each static inline symbol drops/change the inline score/cost of everything above and below it. tmps_grow_p(ix) is the only function call in this macro, and its light weight with only 1/2 args, and a useful retval. Upper case things in Perl API indicate "fast" or "lightweight" operations. lower-case will make everyone think its some heavyweight function. Not every P5P dev will grep/code expansion explorer the back end of every last macro/token/func they type in C. By being a macro PUSH_EXTEND_MORTAL__SV_C, the SvTEMP_on() is guaranteed to fuse with the caller/enclosing frame's SvIOK_on()/SvNOK_on()/SvPOK_on() statement right above or right below. Making it a static inline greatly reduces the chance of fusing 2 ; statements into 1 constant fold. ; ; becomes ; } ; and the } is a link able symbol boundary on the C compiler's AST of grammer/bytecode/RTL/Asm nodes, so thats alot "deeper" the CC/LTOer has to walk and search for common or duplicate C grammer operators. Will all production C compilers, even with LTO turned off, or -O1 or partial optimization (hypothetically, a CC project can emit slightly less efficient machine code if direct to do -O2 with C dbg symbols code vs -O2 no C dbg symbols).

Example from 2022 2006256 The author didn't use Perl's STRLENs() macro, the author did a sizeof(const char *)-1, which shows the author doesn't know what behavior/Type/code is behind those long UPPERCASE tokens.

I'd prefer this macro to be left alone. The only thing that really needs to be rethinked is, if it should be renamed to a shorter/better macro name, and marked as CPAN Public API. Note sv_2mortal() does more work than this macro, and some of that work is protecting against shoddy CPAN/Private XS code. Pros and Cons of marking it public API, or only using it secretly inside a future newmSViv() or mNewSViv() going forward.

@mauke
Copy link
Contributor Author

mauke commented May 9, 2025

Bad idea, each static inline symbol drops/change the inline score/cost of everything above and below it.

I don't see how that's relevant.

tmps_grow_p(ix) is the only function call in this macro, and its light weight with only 1/2 args, and a useful retval.

I don't see how that's relevant.

Upper case things in Perl API indicate "fast" or "lightweight" operations. lower-case will make everyone think its some heavyweight function.

I don't think that's true (for example, LEAVE is as heavy as it gets), but I don't see how it is relevant either, as this function isn't API.

Not every P5P dev will grep/code expansion explorer the back end of every last macro/token/func they type in C.

I don't see how that's relevant.

By being a macro PUSH_EXTEND_MORTAL__SV_C, the SvTEMP_on() is guaranteed to fuse with the caller/enclosing frame's SvIOK_on()/SvNOK_on()/SvPOK_on() statement right above or right below.

False.

Making it a static inline greatly reduces the chance of fusing 2 ; statements into 1 constant fold. ; ; becomes ; } ; and the } is a link able symbol boundary on the C compiler's AST of grammer/bytecode/RTL/Asm nodes, so thats alot "deeper" the CC/LTOer has to walk and search for common or duplicate C grammer operators.

This is hard to parse. In particular, "fusing 2 ; statements into 1 constant fold", "link able symbol boundary", and "AST of grammer/bytecode/RTL/Asm nodes" are borderline gibberish. Note that this patch neither adds nor removes } symbols.

Will all production C compilers, even with LTO turned off, or -O1 or partial optimization (hypothetically, a CC project can emit slightly less efficient machine code if direct to do -O2 with C dbg symbols code vs -O2 no C dbg symbols).

This is not even a sentence. (And why bring up LTO for a private/internal function?)

Example from 2022 2006256 The author didn't use Perl's STRLENs() macro, the author did a sizeof(const char *)-1, which shows the author doesn't know what behavior/Type/code is behind those long UPPERCASE tokens.

Example of what? (Again, how is this relevant?)

Also, nowhere in that commit is there a sizeof(const char *) or a -1. The latter is the actual bug: C_ARRAY_END works fine on string literals; it just gives you a pointer one past the end of the char data (including the NUL byte) instead of a pointer to the NUL byte (as the original code expected). Assuming the compiler merges identical string literals (which I admit is not guaranteed), legal_paired_opening_delims_end = C_ARRAY_END(EXTRA_OPENING_UTF8_BRACKETS) - 1; would have been slightly confusing, but correct.

I'd prefer this macro to be left alone. The only thing that really needs to be rethinked is, if it should be renamed to a shorter/better macro name, and marked as CPAN Public API.

Irrelevant for this PR.

Note sv_2mortal() does more work than this macro, and some of that work is protecting against shoddy CPAN/Private XS code.

I don't see how that's relevant.

Pros and Cons of marking it public API, or only using it secretly inside a future newmSViv() or mNewSViv() going forward.

This is not a sentence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defer-next-dev This PR should not be merged yet, but await the next development cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants