Weed out needless PERL_UNUSED_ARG #14420
Comments
From jasmine.ngan@outlook.comHi there, This is my first time contributing a patch so I'm looking for a little guidance. As suggested by the subject and with assistance from Hugo at the London Perl Hackday, I've weeded out some obvious PERL_UNUSED_ARG statements inside static functions as requested in the Perl 5 todo.pod file. The remaining PERL_UNUSED_ARG statements appear to be for variables that are (1) contained in non-static functions, (2) conditionally used depending on pre-processor macros or (3) placeholders so that the static function prototypes match particular function pointers. I'm not sure if this task is completed yet so I haven't removed the item from todo.pm yet. Let me know what you think. Thanks! Kind regards,
|
From jasmine.ngan@outlook.com0001-Removed-PERL_UNUSED_ARG-in-S_grok_bslash_x.patchFrom f4e4e2e0a53f02a32e7cc0d98c62910e979a8a76 Mon Sep 17 00:00:00 2001
From: Jasmine Ngan <jasmine.ngan@outlook.com>
Date: Sat, 17 Jan 2015 14:26:02 +0000
Subject: [PATCH 1/3] Removed PERL_UNUSED_ARG() in S_grok_bslash_x()
output_warning is actually used.
---
dquote_static.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/dquote_static.c b/dquote_static.c
index 16227c1..076e6c2 100644
--- a/dquote_static.c
+++ b/dquote_static.c
@@ -218,8 +218,6 @@ S_grok_bslash_x(pTHX_ char **s, UV *uv, const char** error_msg,
PERL_ARGS_ASSERT_GROK_BSLASH_X;
- PERL_UNUSED_ARG(output_warning);
-
assert(**s == 'x');
(*s)++;
--
1.7.10.4
|
From jasmine.ngan@outlook.com0002-Changed-PERL_UNUSED_VAR-to-PERL_UNUSED_ARG-for-items.patchFrom f80dd918e4d16399482fc7fbefdc357271c6854e Mon Sep 17 00:00:00 2001
From: Jasmine Ngan <jasmine.ngan@outlook.com>
Date: Sat, 17 Jan 2015 15:04:03 +0000
Subject: [PATCH 2/3] Changed PERL_UNUSED_VAR to PERL_UNUSED_ARG for items
items is not a function argument but a local variable.
---
perl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perl.c b/perl.c
index bb4a342..3f32d6e 100644
--- a/perl.c
+++ b/perl.c
@@ -1768,7 +1768,7 @@ S_Internals_V(pTHX_ CV *cv)
# endif
;
PERL_UNUSED_ARG(cv);
- PERL_UNUSED_ARG(items);
+ PERL_UNUSED_VAR(items);
EXTEND(SP, entries);
--
1.7.10.4
|
From jasmine.ngan@outlook.com0003-Added-myself-to-AUTHORS.patchFrom bf3ec1f50e0d089e80dae92aa6fbee774cd91cc6 Mon Sep 17 00:00:00 2001
From: Jasmine Ngan <jasmine.ngan@outlook.com>
Date: Sat, 17 Jan 2015 16:02:44 +0000
Subject: [PATCH 3/3] Added myself to AUTHORS
---
AUTHORS | 1 +
1 file changed, 1 insertion(+)
diff --git a/AUTHORS b/AUTHORS
index 7f2b8f0..61a9dfc 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -534,6 +534,7 @@ Jared Rhine <jared@organic.com>
Jari Aalto <jari.aalto@poboxes.com>
Jarkko Hietaniemi <jhi@iki.fi>
Jasmine Ahuja <jasmine.ahuja11@gmail.com>
+Jasmine Ngan <jasmine.ngan@outlook.com>
Jason A. Smith <smithj4@rpi.edu>
Jason E. Stewart <jason@openinformatics.com>
Jason Hord <pravus@cpan.org>
--
1.7.10.4
|
From @bulk88On Sat Jan 17 09:00:57 2015, jasmine.ngan@outlook.com wrote:
Reading about dquote_static.c and grok_bslash_x brings up a couple questions how its implementation. grok_bslash_x is 0x133 bytes of VC 2003 -O1 32 bit machine code. It is declared as PERL_STATIC_INLINE in dquote_static.c and in "EMiR" embed.fnc. Something that fat in machine code and pagefuls of C code should be inlined in exactly one scenario, it has only 1 caller, and it is unconditionally (basically not in a "if" branch) executed in that caller. VC optimizer refused to inline grok_bslash_x and instead it is called from 3 callers in VC machine code and in Perl C source code, S_regatom, S_regclass, and S_scan_const. The inline came from http://perl5.git.perl.org/perl.git/commitdiff/a04812933fd16241c831d020a4c64870d7ca8624 by khw from 5.17.1. If I didn't have VC's WPO/LTO/LTCG mode turned on, there would be 2 copies of S_grok_bslash_x in the perl binary (VC 6 doesn't have LTCG, too old, so it suffers too), since a normal C linker can't dedup nameless, symbol-less static functions. Here is a symbol list from HPUX HP C blead perl showing that HPUX ld didn't dedup the functions. name addr length I assume that all 3 S_grok_blash* functions are used in re::* XS module. There are 2 things that should have been done. Either S_grok* becomes Perl_grok*_*p and Xp or stays as S_grok* and becomes X in embed.fnc, so they are exported to re::* XS SO. or dquote_static.c becomes a formal translation unit for core but its functions are not exported in embed.fnc. Then on restricted visibility platforms (IE Win32) it is then #include-ed in re::* again but this time S_grok_bslash* are PERL_STATIC_INLINE. dquote_static.c and embed.fnc has an #if PERL_EXT && RESTRICTED_SYM_VIS #if PERL_EXT && RESTRICTED_SYM_VIS This way core's S_grok_bslash can get WPO optimized (random register calling conventions, removing unused params, merging params, etc) but re:: XS SO/DLL gets its own machine code copy that also was hopefully WPO-ed. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Sat Jan 17 09:00:57 2015, jasmine.ngan@outlook.com wrote:
Im inclined to say https://rt-archive.perl.org/perl5/Ticket/Attachment/1326848/707965/0001-Removed-PERL_UNUSED_ARG-in-S_grok_bslash_x.patch is wrong. output_warning is unused in non-DEBUGGING and DEBUGGING. I removed it as an experiment and both build types compiled, see attached patch. -- |
From @bulk880001-WIP-output_warning-doesn-t-seem-to-be-used-in-grok_b.patchFrom 2b0705617ed787a039499f8ae7be530982e65302 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sat, 17 Jan 2015 17:53:18 -0500
Subject: [PATCH] WIP output_warning doesn't seem to be used in grok_bslash_x
not for blead, this patch is an experiment
---
dquote_static.c | 8 +++-----
embed.fnc | 3 +--
embed.h | 2 +-
perl.h | 4 ++++
proto.h | 4 ++--
5 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/dquote_static.c b/dquote_static.c
index 5fe7f0b..505e83b 100644
--- a/dquote_static.c
+++ b/dquote_static.c
@@ -180,8 +180,8 @@ S_grok_bslash_o(pTHX_ char **s, UV *uv, const char** error_msg,
}
PERL_STATIC_INLINE bool
-S_grok_bslash_x(pTHX_ char **s, UV *uv, const char** error_msg,
- const bool output_warning, const bool strict,
+S_grok_bslash_xp(pTHX_ char **s, UV *uv, const char** error_msg,
+ const bool strict,
const bool silence_non_portable,
const bool UTF)
{
@@ -216,9 +216,7 @@ S_grok_bslash_x(pTHX_ char **s, UV *uv, const char** error_msg,
STRLEN numbers_len;
I32 flags = PERL_SCAN_DISALLOW_PREFIX;
- PERL_ARGS_ASSERT_GROK_BSLASH_X;
-
- PERL_UNUSED_ARG(output_warning);
+ PERL_ARGS_ASSERT_GROK_BSLASH_XP;
assert(**s == 'x');
(*s)++;
diff --git a/embed.fnc b/embed.fnc
index 1444fd0..0652717 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -799,9 +799,8 @@ EMsR |bool |grok_bslash_o |NN char** s|NN UV* uv \
|const bool strict \
|const bool silence_non_portable \
|const bool utf8
-EMiR |bool |grok_bslash_x |NN char** s|NN UV* uv \
+EMiR |bool |grok_bslash_xp |NN char** s|NN UV* uv \
|NN const char** error_msg \
- |const bool output_warning \
|const bool strict \
|const bool silence_non_portable \
|const bool utf8
diff --git a/embed.h b/embed.h
index 875de9c..f259a33 100644
--- a/embed.h
+++ b/embed.h
@@ -1037,7 +1037,7 @@
#define form_short_octal_warning(a,b) S_form_short_octal_warning(aTHX_ a,b)
#define grok_bslash_c(a,b) S_grok_bslash_c(aTHX_ a,b)
#define grok_bslash_o(a,b,c,d,e,f,g) S_grok_bslash_o(aTHX_ a,b,c,d,e,f,g)
-#define grok_bslash_x(a,b,c,d,e,f,g) S_grok_bslash_x(aTHX_ a,b,c,d,e,f,g)
+#define grok_bslash_xp(a,b,c,d,e,f) S_grok_bslash_xp(aTHX_ a,b,c,d,e,f)
#define regcurly S_regcurly
# endif
# if defined(PERL_IN_REGCOMP_C) || defined(PERL_IN_UTF8_C)
diff --git a/perl.h b/perl.h
index 046bbdb..3b1437b 100644
--- a/perl.h
+++ b/perl.h
@@ -6294,6 +6294,10 @@ int flock(int fd, int op);
/* Output flags: */
#define PERL_SCAN_GREATER_THAN_UV_MAX 0x02 /* should this merge with above? */
+#define grok_bslash_x(s, uv, error_msg, output_warning, strict, silence_non_portable, UTF) \
+ S_grok_bslash_xp(aTHX_ s, uv, error_msg, strict, silence_non_portable, UTF)
+
+
/* to let user control profiling */
#ifdef PERL_GPROF_CONTROL
extern void moncontrol(int);
diff --git a/proto.h b/proto.h
index e09c4d3..0f75eda 100644
--- a/proto.h
+++ b/proto.h
@@ -7354,12 +7354,12 @@ STATIC bool S_grok_bslash_o(pTHX_ char** s, UV* uv, const char** error_msg, cons
#define PERL_ARGS_ASSERT_GROK_BSLASH_O \
assert(s); assert(uv); assert(error_msg)
-PERL_STATIC_INLINE bool S_grok_bslash_x(pTHX_ char** s, UV* uv, const char** error_msg, const bool output_warning, const bool strict, const bool silence_non_portable, const bool utf8)
+PERL_STATIC_INLINE bool S_grok_bslash_xp(pTHX_ char** s, UV* uv, const char** error_msg, const bool strict, const bool silence_non_portable, const bool utf8)
__attribute__warn_unused_result__
__attribute__nonnull__(pTHX_1)
__attribute__nonnull__(pTHX_2)
__attribute__nonnull__(pTHX_3);
-#define PERL_ARGS_ASSERT_GROK_BSLASH_X \
+#define PERL_ARGS_ASSERT_GROK_BSLASH_XP \
assert(s); assert(uv); assert(error_msg)
PERL_STATIC_INLINE I32 S_regcurly(const char *s)
--
1.7.9.msysgit.0
|
From @hvds"bulk88 via RT" <perlbug-followup@perl.org> wrote: I suggest making a separate ticket for that, I don't think it relates to Hugo |
From @hvds"bulk88 via RT" <perlbug-followup@perl.org> wrote: How is that managing to compile line 226, just after the context lines Hugo |
From @bulk88hv@crypt.org wrote:
My patch worked on blead from Jan 6 2015/ var output_warning really was unused on that day PERL_STATIC_INLINE bool /* Documentation to be supplied when interface nailed down finally PERL_ARGS_ASSERT_GROK_BSLASH_X; PERL_UNUSED_ARG(output_warning); assert(**s == 'x'); if (strict) { if (**s != '{') { *uv = grok_hex(*s, &len, &flags, NULL); e = strchr(*s, '}'); (*s)++; /* Point to expected first digit (could be first byte of flags |= PERL_SCAN_ALLOW_UNDERSCORES; *uv = grok_hex(*s, &numbers_len, &flags, NULL); if (strict && numbers_len != (STRLEN) (e - *s)) { /* Return past the '}' */ return TRUE; |
From @hvdsbulk88 <bulk88@hotmail.com> wrote: Ah, the reference was newly added by khw in 879eb60 a week ago: Hugo |
From @iabynOn Sat, Jan 17, 2015 at 09:00:57AM -0800, Jasmine Ngan wrote:
Thanks, all applied as commit d3db151. Sorry for the delay, your ticket
That's probably as much as can be done for now.
I think this is more of a continuous task. Perhaps we should just note in -- |
Migrated from rt.perl.org#123616 (status was 'open')
Searchable as RT123616$
The text was updated successfully, but these errors were encountered: