perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

S_new_SV: mark args unused, make inline, correct typo

Open richardleach opened this issue 1 year ago • 4 comments

When sv_inline.h was created in https://github.com/Perl/perl5/commit/75acd14e43f2ffb698fc7032498f31095b56adb5 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.

richardleach avatar May 11 '24 14:05 richardleach

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.

tonycoz avatar May 13 '24 00:05 tonycoz

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?

iabyn avatar May 20 '24 10:05 iabyn

Yes it should have an embed.fnc entry

khwilliamson avatar May 20 '24 11:05 khwilliamson

Ok, will work on that.

richardleach avatar May 21 '24 10:05 richardleach

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

jkeenan avatar Jun 10 '24 20:06 jkeenan

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.

tonycoz avatar Jun 10 '24 23:06 tonycoz