Skip to content
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

S_new_SV: mark args unused, make inline, correct typo #22208

Merged

Conversation

richardleach
Copy link
Contributor

When sv_inline.h was created in
75acd14
and a number of things moved into it, the S_new_SV debugging function should
have been made PERL_STATIC_INLINE. This commit now does that.It also marks
the arguments to S_new_SV as PERL_UNUSED_ARG, reducing warnings on some
builds, and corrects a transcription error in the FUNCTION argument.

The first two changes were suggested in #22125 and the third noticed during
preparation of this PR.

sv_inline.h Outdated Show resolved Hide resolved
@richardleach richardleach force-pushed the hydahy/22125_leaking_scalars_inline branch from b3717a0 to 250be70 Compare May 11, 2024 17:19
@tonycoz
Copy link
Contributor

tonycoz commented May 13, 2024

Something that I should have picked up from the original commit: S_new_SV() is visible outside of perl itself, and like the other visible inline functions it should use a Perl_ prefix.

Looks fine otherwise.

@richardleach richardleach force-pushed the hydahy/22125_leaking_scalars_inline branch from 250be70 to 1c16f24 Compare May 13, 2024 20:37
@iabyn
Copy link
Contributor

iabyn commented May 20, 2024

I think S_new_SV() should be made always defined, not just existing on DEBUG_LEAKING_SCALARS builds; but with the extra sv->sv_debug* and logging lines wrapped in ifdef DEBUG_LEAKING_SCALARS. Then the macro new_SV() should stop being a multi-line macro and just become a call to the inline function S_new_SV(). Then the code comment "provide a real function for a debugger to play with" becomes redundant.

The weird "sometimes a macro, sometimes a function" arrangement was put in place by me back before we could use inline functions.

It should probably also have an entry in embed.fnc?

@khwilliamson
Copy link
Contributor

Yes it should have an embed.fnc entry

@richardleach
Copy link
Contributor Author

Ok, will work on that.

@richardleach richardleach force-pushed the hydahy/22125_leaking_scalars_inline branch from 1c16f24 to fdbd16f Compare May 22, 2024 23:05
sv_inline.h Show resolved Hide resolved
@richardleach richardleach force-pushed the hydahy/22125_leaking_scalars_inline branch 2 times, most recently from ddecdf3 to ececbcf Compare May 23, 2024 21:49
@jkeenan
Copy link
Contributor

jkeenan commented Jun 10, 2024

@smpeters, @mauke, @iabyn, @tonycoz Can we get an update on the status of this pull request? thanks.

@tonycoz
Copy link
Contributor

tonycoz commented Jun 10, 2024

The name defined in embed.fnc is new_sv while the name used in the code is new_SV.

You've also put the name in embed.fnc within a #if defined(PERL_IN_SV_C) block, so the declaration isn't visible to most code, which together with the name mismatch resulted in:

cc -c -DPERL_CORE -D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c99 -O2 -Wall -Werror=pointer-arith -Werror=vla -Wextra -Wno-long-long -Wno-declaration-after-statement -Wc++-compat -Wwrite-strings sv.c
In file included from perl.h:6186,
                 from sv.c:32:
proto.h:9141:1: warning: ‘Perl_new_sv’ declared ‘static’ but never defined [-Wunused-function]
 9141 | Perl_new_sv(pTHX_ const char *file, int line, const char *func);
      | ^~~~~~~~~~~

You'll probably need to add an o flag in embed.fnc to prevent the default new_SV() macro that will expect the file, line and function parameters.

When sv_inline.h was created in
Perl@75acd14
and a number of things moved into it, the S_new_SV debugging function should
have been made PERL_STATIC_INLINE and given the Perl_ prefix. This commit
now does those things. It also marks the arguments to Perl_new_SV as
PERL_UNUSED_ARG, reducing warnings on some builds.

Additionally, now that we can use inline functions, the new_SV() macro is
now just a call to this inline function, rather than being that only on
DEBUG_LEAKING_SCALARS builds and a multi-line macro the rest of the time.
@richardleach richardleach force-pushed the hydahy/22125_leaking_scalars_inline branch from ececbcf to 516f7c8 Compare June 11, 2024 20:17
@richardleach richardleach merged commit c79fe2b into Perl:blead Jun 12, 2024
29 checks passed
@richardleach richardleach deleted the hydahy/22125_leaking_scalars_inline branch June 12, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants