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

[PATCH] remove PL_patchlevel from and optimize S_minus_v #12687

Open
p5pRT opened this issue Jan 4, 2013 · 39 comments
Open

[PATCH] remove PL_patchlevel from and optimize S_minus_v #12687

p5pRT opened this issue Jan 4, 2013 · 39 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Jan 4, 2013

Migrated from rt.perl.org#116296 (status was 'open')

Searchable as RT116296$

@p5pRT
Copy link
Author

p5pRT commented Jan 4, 2013

From @bulk88

See attached patch.

@p5pRT
Copy link
Author

p5pRT commented Jan 4, 2013

From @bulk88

0001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patch
From 7e84c731682a55cf3f8f88caa599c844c8151b8f Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 4 Jan 2013 12:34:33 -0500
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v

|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.

Prior to this patch, S_minus_v was 0x129 bytes long of x86-32 VC 2003 -O1
-GL machine code. After it is 0x70 bytes long. Although S_minus_v is not
hot and is rarely used, making the interp image smaller, and therefore
faster is a good idea. There should not be any user visible changes to -v
with this patch. switches.t contains a regexp for -v already so no further
tests were added.
---
 perl.c |  102 ++++++++++++++++++++++++++++++++--------------------------------
 1 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/perl.c b/perl.c
index 0cfb73c..7589e1f 100644
--- a/perl.c
+++ b/perl.c
@@ -3414,32 +3414,38 @@ Perl_moreswitches(pTHX_ const char *s)
 STATIC void
 S_minus_v(pTHX)
 {
+
 	PerlIO * PIO_stdout;
-	if (!sv_derived_from(PL_patchlevel, "version"))
-	    upg_version(PL_patchlevel, TRUE);
 #if !defined(DGUX)
 	{
-	    SV* level= vstringify(PL_patchlevel);
+	    const char * const level_str = "v" PERL_VERSION_STRING;
+	    const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
 #ifdef PERL_PATCHNUM
+	    SV* level;
 #  ifdef PERL_GIT_UNCOMMITTED_CHANGES
-	    SV *num = newSVpvs(PERL_PATCHNUM "*");
+	    static const char num [] = PERL_PATCHNUM "*";
 #  else
-	    SV *num = newSVpvs(PERL_PATCHNUM);
+	    static const char num [] = PERL_PATCHNUM;
 #  endif
 	    {
-		STRLEN level_len, num_len;
-		char * level_str, * num_str;
-		num_str = SvPV(num, num_len);
-		level_str = SvPV(level, level_len);
-		if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
-		    SvREFCNT_dec(level);
-		    level= num;
+		const STRLEN num_len = sizeof(num)-1;
+		/* A very advanced compiler would fold away the strnEQ
+		   and this whole conditional, but most (all?) won't do it.
+		   SV level could also be replaced by with preprocessor
+		   catenation.
+		*/
+		if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+		    /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+		       of the interp so it might contain format characters
+		    */
+		    level = Perl_newSVpvf_nocontext("%s", num);
 		} else {
-		    Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
-		    SvREFCNT_dec(num);
+		    level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
 		}
 	    }
- #endif
+#else
+	SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
 	PIO_stdout =  PerlIO_stdout();
 	    PerlIO_printf(PIO_stdout,
 		"\nThis is perl "	STRINGIFY(PERL_REVISION)
@@ -3447,14 +3453,13 @@ S_minus_v(pTHX)
 		", subversion "		STRINGIFY(PERL_SUBVERSION)
 		" (%"SVf") built for "	ARCHNAME, level
 		);
-	    SvREFCNT_dec(level);
+	    SvREFCNT_dec_NN(level);
 	}
 #else /* DGUX */
 	PIO_stdout =  PerlIO_stdout();
 /* Adjust verbose output as in the perl that ships with the DG/UX OS from EMC */
 	PerlIO_printf(PIO_stdout,
-		Perl_form(aTHX_ "\nThis is perl, %"SVf"\n",
-		    SVfARG(vstringify(PL_patchlevel))));
+		Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n");
 	PerlIO_printf(PIO_stdout,
 			Perl_form(aTHX_ "        built under %s at %s %s\n",
 					OSNAME, __DATE__, __TIME__));
@@ -3472,47 +3477,42 @@ S_minus_v(pTHX)
 #endif
 
 	PerlIO_printf(PIO_stdout,
-		      "\n\nCopyright 1987-2012, Larry Wall\n");
-#ifdef MSDOS
-	PerlIO_printf(PIO_stdout,
-		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
-#endif
-#ifdef DJGPP
-	PerlIO_printf(PIO_stdout,
+		      "\n\nCopyright 1987-2012, Larry Wall\n"
+#if defined(MSDOS)
+		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
+#elif defined(DJGPP)
 		      "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
-		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
-#endif
-#ifdef OS2
-	PerlIO_printf(PIO_stdout,
+		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
+#elif defined(OS2)
 		      "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
-		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
-#endif
-#ifdef OEMVS
-	PerlIO_printf(PIO_stdout,
-		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
-#endif
-#ifdef __VOS__
-	PerlIO_printf(PIO_stdout,
-		      "Stratus VOS port by Paul.Green@stratus.com, 1997-2002\n");
-#endif
-#ifdef POSIX_BC
-	PerlIO_printf(PIO_stdout,
-		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
-#endif
-#ifdef UNDER_CE
-	PerlIO_printf(PIO_stdout,
+		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
+#elif defined(__BEOS__)
+		      "BeOS port Copyright Tom Spindler, 1997-1999\n"
+#elif defined(OEMVS)
+		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
+#elif defined(__VOS__)
+		      "Stratus VOS port by Paul.Green@stratus.com, 1997-2002\n"
+#elif defined(POSIX_BC)
+		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+/* UNDER_CE closes the printf call*/
+#elif defined(UNDER_CE)
 			"WINCE port by Rainer Keuchel, 2001-2002\n"
 			"Built on " __DATE__ " " __TIME__ "\n\n");
 	wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
-	PerlIO_printf(PIO_stdout,
-		      "Symbian port by Nokia, 2004-2005\n");
-#endif
+#elif defined(__SYMBIAN32__)
+		      "Symbian port by Nokia, 2004-2005\n"
+#endif /* end of additional copyright lines */
+
 #ifdef BINARY_BUILD_NOTICE
+#  ifndef UNDER_CE
+	); /* close Larry Wall copyright printf */
+#  endif
 	BINARY_BUILD_NOTICE;
-#endif
 	PerlIO_printf(PIO_stdout,
+#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */
+	PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
 		      "\n\
 Perl may be copied only under the terms of either the Artistic License or the\n\
 GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2013

From @tonycoz

On Fri Jan 04 09​:37​:10 2013, bulk88 wrote​:

See attached patch.

This patch no longer applies.

I can see a couple of places it could be improved​:

+ level = Perl_newSVpvf_nocontext("%s", num);

Couldn't this just be​:

  level = newSVpvn(num, num_len);

  PerlIO_printf(PIO_stdout,
- Perl_form(aTHX_ "\nThis is perl, %"SVf"\n",
- SVfARG(vstringify(PL_patchlevel))));
+ Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n");

Why is Perl_form() being called here?

#elif defined(UNDER_CE)
  "WINCE port by Rainer Keuchel, 2001-2002\n"
  "Built on " __DATE__ " " __TIME__ "\n\n");
  wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#elif defined(__SYMBIAN32__)
+ "Symbian port by Nokia, 2004-2005\n"
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
  BINARY_BUILD_NOTICE;
-#endif
  PerlIO_printf(PIO_stdout,
+#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */
+ PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif

The contortions to handle UNDER_CE ending the PerlIO_printf() are
confusing. Wouldn't it be better to put the wce_hitreturn() in it's own
#ifdef UNDER_CE ?

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2013

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2013

From @bulk88

On Fri Jul 05 00​:01​:09 2013, tonyc wrote​:

On Fri Jan 04 09​:37​:10 2013, bulk88 wrote​:

See attached patch.

This patch no longer applies.

I'll have to work on it then.

I can see a couple of places it could be improved​:

+ level = Perl_newSVpvf_nocontext("%s", num);

Couldn't this just be​:

level = newSVpvn(num, num_len);

No for code size reasons. There is a 2nd Perl_newSVpvf_nocontext below
in the other branch. Compiler will put params on C stack then jump to
the "call Perl_newSVpvf_nocontext" instruction. The "call
Perl_newSVpvf_nocontext" is shared by both branches.
Perl_newSVpvf_nocontext takes 2 params, newSVpvn takes 3 params on ithreads.

 PerlIO\_printf\(PIO\_stdout\,

- Perl_form(aTHX_ "\nThis is perl, %"SVf"\n",
- SVfARG(vstringify(PL_patchlevel))));
+ Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n");

Why is Perl_form() being called here?

Good catch. It is redundant.

#elif defined(UNDER_CE)
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#elif defined(__SYMBIAN32__)
+ "Symbian port by Nokia, 2004-2005\n"
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
BINARY_BUILD_NOTICE;
-#endif
PerlIO_printf(PIO_stdout,
+#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */
+ PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif

The contortions to handle UNDER_CE ending the PerlIO_printf() are
confusing. Wouldn't it be better to put the wce_hitreturn() in it's own
#ifdef UNDER_CE ?

So confusing I need more time to think up an answer. I have to try to
get this to apply to blead so I will figure out what can be done about
UNDER_CE when I try to compile the code.

Tony

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2013

From @bulk88

Patch rewritten for blead. Some changes from the last old one is the
LOCAL_PATCH_COUNT stuff was changed, and I tried to minimize diff size
by keeping the "#ifdef OSNAME #endif #ifdef OSNAME #endif" tree the
same. In old patch the OS names were rearranged into "#if #elif #elif"
tree. Also the whitespace/indents on the double quote lines was kept the
same even if it was wrong. Symbian and Win CE conditionals were swapped
in new patch to make more sense. The new LOCAL_PATCH_COUNT code is a
compromise to not put the GPL and copyright strings into macros and
having the GPL and copyright literals appear 3 times in preprocessed
output (only the C compiler can pick which string to use since
LOCAL_PATCH_COUNT isn't a preprocessor constant and takes the size of a
struct in its expansion from patchlevel.h).

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2013

From @bulk88

0001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patch
From 4acb8e4ecbbd1691ad8e83c1a8a8e57a94fd97d3 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 5 Jul 2013 19:12:29 -0400
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v

|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.

Prior to this patch, S_minus_v was 0x13b bytes long of x86-32 VC 2003 -O1
-GL machine code. After it is 0x77 bytes long. Although S_minus_v is not
hot and is rarely used, making the interp image smaller, and therefore
faster is a good idea. There should not be any user visible changes to -v
with this patch. switches.t contains a regexp for -v already so no further
tests were added.
---
 perl.c |   95 ++++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/perl.c b/perl.c
index bad66f5..2198205 100644
--- a/perl.c
+++ b/perl.c
@@ -3502,30 +3502,35 @@ STATIC void
 S_minus_v(pTHX)
 {
 	PerlIO * PIO_stdout;
-	if (!sv_derived_from(PL_patchlevel, "version"))
-	    upg_version(PL_patchlevel, TRUE);
 	{
-	    SV* level= vstringify(PL_patchlevel);
+	    const char * const level_str = "v" PERL_VERSION_STRING;
+	    const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
 #ifdef PERL_PATCHNUM
+	    SV* level;
 #  ifdef PERL_GIT_UNCOMMITTED_CHANGES
-	    SV *num = newSVpvs(PERL_PATCHNUM "*");
+	    static const char num [] = PERL_PATCHNUM "*";
 #  else
-	    SV *num = newSVpvs(PERL_PATCHNUM);
+	    static const char num [] = PERL_PATCHNUM;
 #  endif
 	    {
-		STRLEN level_len, num_len;
-		char * level_str, * num_str;
-		num_str = SvPV(num, num_len);
-		level_str = SvPV(level, level_len);
-		if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
-		    SvREFCNT_dec(level);
-		    level= num;
+		const STRLEN num_len = sizeof(num)-1;
+		/* A very advanced compiler would fold away the strnEQ
+		   and this whole conditional, but most (all?) won't do it.
+		   SV level could also be replaced by with preprocessor
+		   catenation.
+		*/
+		if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+		    /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+		       of the interp so it might contain format characters
+		    */
+		    level = Perl_newSVpvf_nocontext("%s", num);
 		} else {
-		    Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
-		    SvREFCNT_dec(num);
+		    level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
 		}
 	    }
- #endif
+#else
+	SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
 	PIO_stdout =  PerlIO_stdout();
 	    PerlIO_printf(PIO_stdout,
 		"\nThis is perl "	STRINGIFY(PERL_REVISION)
@@ -3533,59 +3538,61 @@ S_minus_v(pTHX)
 		", subversion "		STRINGIFY(PERL_SUBVERSION)
 		" (%"SVf") built for "	ARCHNAME, level
 		);
-	    SvREFCNT_dec(level);
+	    SvREFCNT_dec_NN(level);
 	}
-#if defined(LOCAL_PATCH_COUNT)
-	if (LOCAL_PATCH_COUNT > 0)
-	    PerlIO_printf(PIO_stdout,
-			  "\n(with %d registered patch%s, "
-			  "see perl -V for more detail)",
-			  LOCAL_PATCH_COUNT,
-			  (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
-
 	PerlIO_printf(PIO_stdout,
-		      "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+    /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+        LOCAL_PATCH_COUNT == 1 ?
+                "\n(with %d registered patch, see perl -V for more detail)%s"
+        : LOCAL_PATCH_COUNT > 0 ?
+                "\n(with %d registered patches, see perl -V for more detail)%s"
+        : "%.0s%s",
+        LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
+#endif
+		      "\n\nCopyright 1987-2013, Larry Wall\n"
 #ifdef MSDOS
-	PerlIO_printf(PIO_stdout,
-		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
 #endif
 #ifdef DJGPP
-	PerlIO_printf(PIO_stdout,
 		      "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
-		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
 #endif
 #ifdef OS2
-	PerlIO_printf(PIO_stdout,
 		      "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
-		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
 #endif
 #ifdef OEMVS
-	PerlIO_printf(PIO_stdout,
-		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
 #endif
 #ifdef __VOS__
-	PerlIO_printf(PIO_stdout,
-		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
 #endif
 #ifdef POSIX_BC
-	PerlIO_printf(PIO_stdout,
-		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+		      "Symbian port by Nokia, 2004-2005\n"
 #endif
+/* UNDER_CE closes the printf call*/
 #ifdef UNDER_CE
-	PerlIO_printf(PIO_stdout,
 			"WINCE port by Rainer Keuchel, 2001-2002\n"
 			"Built on " __DATE__ " " __TIME__ "\n\n");
 	wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
-	PerlIO_printf(PIO_stdout,
-		      "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
 #ifdef BINARY_BUILD_NOTICE
+#  ifndef UNDER_CE
+	); /* close Larry Wall copyright printf */
+#  endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
 	BINARY_BUILD_NOTICE;
 #endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
 	PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
 		      "\n\
 Perl may be copied only under the terms of either the Artistic License or the\n\
 GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Jul 6, 2013

From @iabyn

On Fri, Jul 05, 2013 at 08​:34​:01AM -0700, bulk88 via RT wrote​:

On Fri Jul 05 00​:01​:09 2013, tonyc wrote​:

I can see a couple of places it could be improved​:

+ level = Perl_newSVpvf_nocontext("%s", num);

Couldn't this just be​:

level = newSVpvn(num, num_len);

No for code size reasons. There is a 2nd Perl_newSVpvf_nocontext below
in the other branch. Compiler will put params on C stack then jump to
the "call Perl_newSVpvf_nocontext" instruction. The "call
Perl_newSVpvf_nocontext" is shared by both branches.
Perl_newSVpvf_nocontext takes 2 params, newSVpvn takes 3 params on ithreads.

I really don't like this part. Its just an excessive level of
micro-optimisation for code that's only called once at most, and will have
a negligible effect on the overall binary size.

On x86_64 platforms with the "first 6 args are in registers" calling
convention, using newSVpvn() will likely involve no more code, since
my_perl will already be in the relevant register; while on x86, its likely
just a push of a register value.

The downside is that the src code becomes more bloated and confusing.

--
Fire extinguisher (n) a device for holding open fire doors.

@p5pRT
Copy link
Author

p5pRT commented Jul 6, 2013

From @bulk88

On Sat Jul 06 07​:45​:05 2013, davem wrote​:

On x86_64 platforms with the "first 6 args are in registers" calling
convention, using newSVpvn() will likely involve no more code, since
my_perl will already be in the relevant register; while on x86, its
likely
just a push of a register value.

On Win64 that is wrong. my_perl has to be copied every time to the 1st
callee register by the caller from the caller's non-vol register that it
saved or caller's c stack auto. There will always be an assignment to
first register (callee's my_perl) on x64 on cdecl for each perl func
call. The calling convention does not say anywhere that a callee must
keep incoming params in registers nonvolatile. What if the callee
doesn't need to every use my_perl again until it returns and calls
strlen which doesn't take a my_perl? The caller just lost their my_perl
pointer.

Keeping func params const on assembly level, to avoid copying in the
caller for each call, would require a machine code JITer and/or random
calling conventions. Random calling conventions are implemented in some
C compilers for Windows, but only for Win32, not Win64.

http​://lists.cs.uiuc.edu/pipermail/llvmdev/2009-September/025927.html

http​://nondot.org/sabre/LLVMNotes/GlobalRegisterVariables.txt

Here is the boot func from re.dll
_______________________________________________________________
0000000010001B20 sub rsp,48h
0000000010001B24 mov rax,qword ptr [rcx+70h]
0000000010001B28 mov rdx,qword ptr [rcx+18h]
0000000010001B2C mov qword ptr [rsp+60h],rbx
0000000010001B31 mov rbx,qword ptr [rcx]
0000000010001B34 mov qword ptr [rsp+68h],rbp
0000000010001B39 mov qword ptr [rsp+40h],rsi <<<<<< save non vol
0000000010001B3E mov qword ptr [rsp+38h],rdi
0000000010001B43 movsxd rdi,dword ptr [rax]
0000000010001B46 add rax,0FFFFFFFFFFFFFFFCh
0000000010001B4A lea r9,[rdx+rdi*8]
0000000010001B4E mov qword ptr [rcx+70h],rax
0000000010001B52 inc edi
0000000010001B54 sub rbx,r9
0000000010001B57 movsxd rbp,edi
0000000010001B5A lea r8,[string "v5.14.0" (1003D378h)]
0000000010001B61 mov rdx,qword ptr [rdx+rbp*8]
0000000010001B65 mov r9d,7
0000000010001B6B mov rsi,rcx <<<<<<<<<<<<<<<<<<<<<<<<<<
incoming param copied to non-vol reg
0000000010001B6E sar rbx,3
0000000010001B72 call Perl_xs_apiversion_bootcheck (1000135Ah)
0000000010001B77 lea r9,[string "0.18" (1003D36Ch)]
0000000010001B7E mov r8d,edi
0000000010001B81 mov edx,ebx
0000000010001B83 mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<<
0000000010001B86 mov qword ptr [rsp+20h],4
0000000010001B8F call Perl_xs_version_bootcheck (10001348h)
0000000010001B94 lea r9,[string "re.c" (1003D380h)]
0000000010001B9B lea r8,[XS_re_install (10001A50h)]
0000000010001BA2 lea rdx,[string "re​::install" (1003D360h)]
0000000010001BA9 mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<<
0000000010001BAC call Perl_newXS (1000134Eh)
0000000010001BB1 lea rax,[string "$" (1003A7DCh)]
0000000010001BB8 lea r9,[string "re.c" (1003D380h)]
0000000010001BBF lea r8,[XS_re_regmust (10001860h)]
0000000010001BC6 lea rdx,[string "re​::regmust" (1003D350h)]
0000000010001BCD mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<<
0000000010001BD0 mov dword ptr [rsp+28h],0
0000000010001BD8 mov qword ptr [rsp+20h],rax
0000000010001BDD call Perl_newXS_flags (10001366h)
0000000010001BE2 mov r8,qword ptr [rsi+5A8h]
0000000010001BE9 mov rdi,qword ptr [rsp+38h]
0000000010001BEE mov rbx,qword ptr [rsp+60h]
0000000010001BF3 test r8,r8
0000000010001BF6 je boot_re+0E3h (10001C03h)
0000000010001BF8 mov edx,dword ptr [rsi+38h]
0000000010001BFB mov rcx,rsi <<<<<<<<<<<<<<<<<<<<<<<<<<
0000000010001BFE call Perl_call_list (10001354h)
0000000010001C03 mov rax,qword ptr [rsi+18h]
0000000010001C07 lea rcx,[rsi+0BF8h]
0000000010001C0E mov qword ptr [rax+rbp*8],rcx
0000000010001C12 mov rax,qword ptr [rsi+18h]
0000000010001C16 lea rcx,[rax+rbp*8]
0000000010001C1A mov rbp,qword ptr [rsp+68h]
0000000010001C1F mov qword ptr [rsi],rcx
0000000010001C22 mov rsi,qword ptr [rsp+40h] <<<<<<<<<restore
non vol
0000000010001C27 add rsp,48h
0000000010001C2B ret
____________________________________________________________

I really don't like this part. Its just an excessive level of
micro-optimisation for code that's only called once at most, and will
have
a negligible effect on the overall binary size.
................
The downside is that the src code becomes more bloated and confusing.

And this is why people give up, switch languages, then tell all their
friends Perl [5 or Perl world] is dead. If the git repo is locked, code
will never become more confusing. The assumption that progress is for
communists and hippies is the death of Perl 5.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 7, 2013

From @khwilliamson

On 07/06/2013 10​:10 AM, bulk88 via RT wrote​:

I really don't like this part. Its just an excessive level of
micro-optimisation for code that's only called once at most, and will
have
a negligible effect on the overall binary size.
................
The downside is that the src code becomes more bloated and confusing.
And this is why people give up, switch languages, then tell all their
friends Perl [5 or Perl world] is dead. If the git repo is locked, code
will never become more confusing. The assumption that progress is for
communists and hippies is the death of Perl 5.

I agree with Dave, and I believe it is counterproductive to write code
based on how current particular hardware works, when that code will long
outlive the hardware and likely the whole underlying mechanism.

But suppose we are wrong; suppose it has to be implemented the way you
did. It's not at all obvious that it has to be that way. Therefore
there should be comments in the code as to your reasoning. If not, the
next guy might come along and not realize that this implementation has
to be this particular way, and change it out. If indeed it is required
to be this way, that might introduce subtle failures that the
accompanying tests don't catch. It's unfair to those who follow to not
comment such subtleties.

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2013

From @tonycoz

On Sat, Jul 06, 2013 at 09​:10​:12AM -0700, bulk88 via RT wrote​:

On Sat Jul 06 07​:45​:05 2013, davem wrote​:

On x86_64 platforms with the "first 6 args are in registers" calling
convention, using newSVpvn() will likely involve no more code, since
my_perl will already be in the relevant register; while on x86, its
likely
just a push of a register value.

On Win64 that is wrong. my_perl has to be copied every time to the 1st
callee register by the caller from the caller's non-vol register that it
saved or caller's c stack auto. There will always be an assignment to
first register (callee's my_perl) on x64 on cdecl for each perl func
call. The calling convention does not say anywhere that a callee must
keep incoming params in registers nonvolatile. What if the callee
doesn't need to every use my_perl again until it returns and calls
strlen which doesn't take a my_perl? The caller just lost their my_perl
pointer.

Keeping func params const on assembly level, to avoid copying in the
caller for each call, would require a machine code JITer and/or random
calling conventions. Random calling conventions are implemented in some
C compilers for Windows, but only for Win32, not Win64.

While Dave was wrong about the registers, his general argument about
the micro-optimization is correct, MSVC isn't the only compiler.

With gcc/clang the change *costs* 5 bytes[1] - while making the code a
little harder to read. Both gcc and clang optimize out the
conditional.

I really don't like this part. Its just an excessive level of
micro-optimisation for code that's only called once at most, and will
have
a negligible effect on the overall binary size.
................
The downside is that the src code becomes more bloated and confusing.

And this is why people give up, switch languages, then tell all their
friends Perl [5 or Perl world] is dead. If the git repo is locked, code
will never become more confusing. The assumption that progress is for
communists and hippies is the death of Perl 5.

I think it's great that you're enthusiastic about optimizing perl's
implementation, but treating a comment about fairly questionable
optimization as an attack on all changes is reaching.

We optimize for several things, generated code size is important, but
not more important than a) developer time - the poor sob who comes
along in 6 months or 5 years and needs to understand the code, b)
performance.

And code size doesn't map one-to-one to performance - several
performance optimizations produce significantly larger code.

Performance doesn't matter much for S_minus_v(), but developer time
matters as much there as it does anywhere else.

Tony

[1] gcc, default Configure set optimization, -Duseithreads,
git_version.h edited to match a tagged release, first
Perl_newSVpvf_no_context​:

00000000000050a0 <S_minus_v>​:
  50a0​: 41 54 push %r12
  50a2​: be 00 00 00 00 mov $0x0,%esi
  50a3​: R_X86_64_32 .rodata+0x8ad
  50a7​: 31 c0 xor %eax,%eax
  50a9​: 55 push %rbp
  50aa​: 48 89 fd mov %rdi,%rbp
  50ad​: bf 00 00 00 00 mov $0x0,%edi
  50ae​: R_X86_64_32 .rodata.str1.1+0x55
  50b2​: 53 push %rbx
  50b3​: e8 00 00 00 00 callq 50b8 <S_minus_v+0x18>
  50b4​: R_X86_64_PC32 Perl_newSVpvf_nocontext+0xfffffffffffffffc

Then newSVpvn()​:

00000000000050a0 <S_minus_v>​:
  50a0​: 41 54 push %r12
  50a2​: 31 d2 xor %edx,%edx
  50a4​: be 00 00 00 00 mov $0x0,%esi
  50a5​: R_X86_64_32 .rodata+0x8ad
  50a9​: 55 push %rbp
  50aa​: 53 push %rbx
  50ab​: 48 89 fb mov %rdi,%rbx
  50ae​: e8 00 00 00 00 callq 50b3 <S_minus_v+0x13>
  50af​: R_X86_64_PC32 Perl_newSVpvn+0xfffffffffffffffc

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2013

From @tonycoz

On Fri, Jul 05, 2013 at 04​:29​:29PM -0700, bulk88 via RT wrote​:

Prior to this patch, S_minus_v was 0x13b bytes long of x86-32 VC 2003 -O1
-GL machine code. After it is 0x77 bytes long. Although S_minus_v is not
hot and is rarely used, making the interp image smaller, and therefore
faster is a good idea. There should not be any user visible changes to -v
with this patch. switches.t contains a regexp for -v already so no further
tests were added.
---
perl.c | 95 ++++++++++++++++++++++++++++++++++-----------------------------
1 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/perl.c b/perl.c
index bad66f5..2198205 100644
--- a/perl.c
+++ b/perl.c
@​@​ -3502,30 +3502,35 @​@​ STATIC void
S_minus_v(pTHX)
{
PerlIO * PIO_stdout;
- if (!sv_derived_from(PL_patchlevel, "version"))
- upg_version(PL_patchlevel, TRUE);
{
- SV* level= vstringify(PL_patchlevel);
+ const char * const level_str = "v" PERL_VERSION_STRING;
+ const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
#ifdef PERL_PATCHNUM
+ SV* level;
# ifdef PERL_GIT_UNCOMMITTED_CHANGES
- SV *num = newSVpvs(PERL_PATCHNUM "*");
+ static const char num [] = PERL_PATCHNUM "*";
# else
- SV *num = newSVpvs(PERL_PATCHNUM);
+ static const char num [] = PERL_PATCHNUM;
# endif
{
- STRLEN level_len, num_len;
- char * level_str, * num_str;
- num_str = SvPV(num, num_len);
- level_str = SvPV(level, level_len);
- if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
- SvREFCNT_dec(level);
- level= num;
+ const STRLEN num_len = sizeof(num)-1;
+ /* A very advanced compiler would fold away the strnEQ
+ and this whole conditional, but most (all?) won't do it.
+ SV level could also be replaced by with preprocessor
+ catenation.
+ */
+ if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+ /* per 46807d8, PERL_PATCHNUM is outside of the control
+ of the interp so it might contain format characters
+ */
+ level = Perl_newSVpvf_nocontext("%s", num);
} else {
- Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
- SvREFCNT_dec(num);
+ level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
}
}
- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */

Except (as discussed) the use of newSVpvn(), I think it's good up to
here.

 PIO\_stdout =  PerlIO\_stdout\(\);
     PerlIO\_printf\(PIO\_stdout\,
     "\\nThis is perl "    STRINGIFY\(PERL\_REVISION\)

@​@​ -3533,59 +3538,61 @​@​ S_minus_v(pTHX)
", subversion " STRINGIFY(PERL_SUBVERSION)
" (%"SVf") built for " ARCHNAME, level
);
- SvREFCNT_dec(level);
+ SvREFCNT_dec_NN(level);

That's fine too.

 \}

-#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
-
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+ LOCAL_PATCH_COUNT == 1 ?
+ "\n(with %d registered patch, see perl -V for more detail)%s"
+ : LOCAL_PATCH_COUNT > 0 ?
+ "\n(with %d registered patches, see perl -V for more detail)%s"
+ : "%.0s%s",
+ LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
+#endif

This code is incorrect.

For some compilers NULL is ((void *)0) - a pointer, not an integer.

Consider - what's the type of the expression​:

LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,

It's also unnecessarily duplicative, in that if the registered patches
note needed to change, it will need to be changed in two places.

gcc and clang complain about the type too, gcc​:

perl.c​: In function ‘S_minus_v’​:
perl.c​:3551​:39​: warning​: pointer/integer type mismatch in conditional expression [enabled by default]
perl.c​:3596​:9​: warning​: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘void *’ [-Wformat]

clang produces a more understandable message​:

perl.c​:3551​:9​: warning​: format specifies type 'int' but the argument has type
  'void *' [-Wformat]
  LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./patchlevel.h​:147​:2​: note​: expanded from macro 'LOCAL_PATCH_COUNT'
  ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
  ^
perl.c​:3551​:9​: warning​: format specifies type 'int' but the argument has type
  'void *' [-Wformat]
  LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./patchlevel.h​:147​:2​: note​: expanded from macro 'LOCAL_PATCH_COUNT'
  ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))

(I think it warns twice because you provide three different format
strings, two of which don't match.)

The original code is clearer I think.

+ "\n\nCopyright 1987-2013, Larry Wall\n"
#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
#endif
#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
#endif
#ifdef OS2
- PerlIO_printf(PIO_stdout,
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
#endif
#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@​stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@​stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+ "Symbian port by Nokia, 2004-2005\n"
#endif
+/* UNDER_CE closes the printf call*/
#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\

Rather than trying to build it all into one printf, I think it would
be more understandable if the printf() started at the Larry Wall line,
and the wce_hitreturn() got its own extra #ifdef UNDER_CE, so​:

PerlIO_printf(PIO_stdout,
  larry's line
#ifdef platform
  platform line
#endif
...
  );
#ifdef UNDER_CE
  wce_hitreturn();
#endif

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2013

From @bulk88

On Sat Jul 06 22​:00​:36 2013, public@​khwilliamson.com wrote​:

I agree with Dave, and I believe it is counterproductive to write code
based on how current particular hardware works, when that code will long
outlive the hardware and likely the whole underlying mechanism.

Does that apply to Perl?

http​://perl5.git.perl.org/perl.git/commit/99ee1dcd0469086e91a96e31a9b9ea27bb7f0c7e

That commit message makes me think you (khw) do believe in writing code
for current hardware.

Some commits (all authors) talk about cache alignment and struct
alignment. Perl already has a bias towards current hardware and not
theoretical hardware from the future, and a strong "there is no C
compiler but GCC bias" (why all the PERL_GCC_BRACE_GROUPS_FORBIDDEN
code?). Arguing over what the future hardware will look like isn't
productive so I won't go down that path. My point is Perl is biased
towards today's processors, and today's OSes, and today's compilers, and
biased towards different ones at the same time. When I hear "your
favorites aren't my favorites so burn in hell", I see that commenter is
wearing rose glasses and it is political/religious BS.

But suppose we are wrong; suppose it has to be implemented the way you
did. It's not at all obvious that it has to be that way. Therefore
there should be comments in the code as to your reasoning. If not, the
next guy might come along and not realize that this implementation has
to be this particular way, and change it out. If indeed it is required
to be this way, that might introduce subtle failures that the
accompanying tests don't catch. It's unfair to those who follow to not
comment such subtleties.

There is a comment that explains what is going on "+/* else CPP
catenation continues from Larry Wall copyright printf */". That explains
the design of what the CPP conditionals are doing and suggests to keep
the CPP concating.

On Sun Jul 07 23​:20​:13 2013, tonyc wrote​:

While Dave was wrong about the registers, his general argument about
the micro-optimization is correct, MSVC isn't the only compiler.

With gcc/clang the change *costs* 5 bytes[1] - while making the code a
little harder to read. Both gcc and clang optimize out the
conditional.

Those are good compilers if they do that then but gcc/clang aren't the
whole world. I will change the "level = Perl_newSVpvf_nocontext("%s",
num);" branch to newSVpvn.

The downside is that the src code becomes more bloated and confusing.

And this is why people give up, switch languages, then tell all their
friends Perl [5 or Perl world] is dead. If the git repo is locked, code
will never become more confusing. The assumption that progress is for
communists and hippies is the death of Perl 5.

I think it's great that you're enthusiastic about optimizing perl's
implementation, but treating a comment about fairly questionable
optimization as an attack on all changes is reaching.

The same opinions are coming out again, self-righteous "if I wouldn't
change it, you have no right to either"
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116443
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115894
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116188

We optimize for several things, generated code size is important, but
not more important than a) developer time - the poor sob who comes
along in 6 months or 5 years and needs to understand the code, b)
performance.

Who is "the poor sob who comes along in 6 months or 5 years and needs to
understand the code"? Commiters are held to no standard of code clarity
or documentation. I've never seen a revert commit to revert something
for lack of comments or clarity. Why is it said by others in the Perl
world (YAPCNA) only a dozen people on earth can hack the C guts?

a recent one
http​://perl5.git.perl.org/perl.git/commitdiff/0c0d42fffa5c2ddd284610f681b65d355471189b
What systems? Tested on any? Seen it in a man page?

http​://perl5.git.perl.org/perl.git/commit/28e464fba839d02f376952199261fc8b7d58a0d8
nothing! get the Ouija board out!

http​://perl5.git.perl.org/perl.git/commit/88675427278c9e6329fba9382072cae9dd00c163
what commit?

http​://perl5.git.perl.org/perl.git/commit/175bc858cac2126947f4b8fa838835417c1430d1
faster? slower?

https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116925 when will
THINKFIRST be documented?

And code size doesn't map one-to-one to performance - several
performance optimizations produce significantly larger code.

Performance doesn't matter much for S_minus_v(), but developer time
matters as much there as it does anywhere else.

S_minus_v never gets changed except for copyright dates. Ideally it
should be a single string written by some .pl script into a .h during
the build process, but that is too complex for me to implement (and will
break every build except Windows if I try), plus removing
BINARY_BUILD_NOTICE and getting AS and DARKPAN to not use it and
introducing a BINARY_BUILD_NOTICE_S macro and "#ifdef
BINARY_BUILD_NOTICE\n#error BINARY_BUILD_NOTICE has been removed, use
BINARY_BUILD_NOTICE_S." is too difficult for me.

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */

Except (as discussed) the use of newSVpvn(), I think it's good up to
here.

Will switch to newSVpvn as said above.

 \}

-#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
-
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor
constant */
+ LOCAL_PATCH_COUNT == 1 ?
+ "\n(with %d registered patch, see perl -V for more
detail)%s"
+ : LOCAL_PATCH_COUNT > 0 ?
+ "\n(with %d registered patches, see perl -V for
more detail)%s"
+ : "%.0s%s",
+ LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
+#endif

This code is incorrect.

For some compilers NULL is ((void *)0) - a pointer, not an integer.

Consider - what's the type of the expression​:

LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,

I knew that would be a portability problem, so the NULL went there.
I didn't want to try to use more complicated features of
sv_vcatpvfn_flags (t or z letter) due to sv_vcatpvfn_flags crashing on
me when using fancy features (bug filed). Google says implicit type
promotion so I guess the size letters have to go in. "%.0s" produces no
output if the param is null, and therefore skips the param. "%.0s" is a
work around to my original idea which became bug #118775.

It's also unnecessarily duplicative, in that if the registered patches
note needed to change, it will need to be changed in two places.

The alternative was more complicated (putting each litteral into a
macro, then putting many instances of each macro). The 2 lines are next
to each other, nobody is likely to change them. If they do, they can see
it on the same screen. 3 seconds of work is nothing.

gcc and clang complain about the type too, gcc​:

perl.c​: In function S_minus_v​:
perl.c​:3551​:39​: warning​: pointer/integer type mismatch in conditional
expression [enabled by default]
perl.c​:3596​:9​: warning​: format %d expects argument of type int,
but argument 3 has type void * [-Wformat]

clang produces a more understandable message​:

perl.c​:3551​:9​: warning​: format specifies type 'int' but the argument
has type
'void *' [-Wformat]
LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./patchlevel.h​:147​:2​: note​: expanded from macro 'LOCAL_PATCH_COUNT'
((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
^
perl.c​:3551​:9​: warning​: format specifies type 'int' but the argument
has type
'void *' [-Wformat]
LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./patchlevel.h​:147​:2​: note​: expanded from macro 'LOCAL_PATCH_COUNT'
((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))

(I think it warns twice because you provide three different format
strings, two of which don't match.)

The original code is clearer I think.

I could break out the LOCAL_PATCH_COUNT stuff into a separate
printf/remove it (as it was originally).

Rather than trying to build it all into one printf, I think it would
be more understandable if the printf() started at the Larry Wall line,
and the wce_hitreturn() got its own extra #ifdef UNDER_CE, so​:

PerlIO_printf(PIO_stdout,
larry's line
#ifdef platform
platform line
#endif
...
);
#ifdef UNDER_CE
wce_hitreturn();
#endif

Tony

The GPL line wont be concatted with the larry CR and platform CR in that
case("> );"). Leaving the only optimization being dec_NN and
concating of larry CR with platform CR.
http​://perl5.git.perl.org/perl.git/commit/e1caacb4fdb62cb28dc825ca0115faf95e569339?f=perl.c
I think the wce_hitreturn (implemented in wince.c) has a screen pager
purpose due to the tiny screen size of a WinCE device, so it has to be
between the larry CR/platform CR and the GPL part on WinCE.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 9, 2013

From @doughera88

On Mon, Jul 08, 2013 at 03​:36​:35PM -0700, "bulk88 via RT" wrote​:

The same opinions are coming out again, self-righteous "if I wouldn't
change it, you have no right to either"
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116443
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115894
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116188

I think that is an unfair characterization of those discussions,
especially when you consider that in two out of the three cases, your
patch was applied.

--
  Andy Dougherty doughera@​lafayette.edu

@p5pRT
Copy link
Author

p5pRT commented Jul 9, 2013

From @rjbs

* bulk88 via RT <perlbug-followup@​perl.org> [2013-07-08T18​:36​:35]

When I hear "your favorites aren't my favorites so burn in hell", I see
that commenter is wearing rose glasses and it is political/religious BS.
[ ⋮ ]
The same opinions are coming out again, self-righteous "if I wouldn't
change it, you have no right to either"

I think that this is not only hyperbole (which is one thing), but also unfair
and, possibly worst of all, not likely to convince anybody of anything helpful.

In every case that I've seen, the issue has not been "I don't care about your
compiler," but "this optimization is only an optimization sometimes, and other
times a cost or no change, but carries a code complexity cost."

Commiters are held to no standard of code clarity or documentation. I've
never seen a revert commit to revert something for lack of comments or
clarity.

I encourage you to politely point out such shortcomings as they occur, to
remind other contributors to improve their comments post facto and to improve
their commit messages next time.

I think it would be in some ways nice, and would have some positive effects, to
have a policy stating that a commiter may not, barring exception circumstances,
apply his own code to blead, but must have a second set of eyes in the form of
another committer who applies the author's branch. Unfortunately, I think that
this is going to be a hard sell, at best.

We need a better culture of code review, I agree. I also agree that it would
benefit everyone were that code review applied not only to code coming from
non-committers, but also within the group of committers. That said, I don't,
of course, think the solution is to have no standards at all.

That's why I hope you will, in the future, politely point out when commits have
been made that are of unclear purpose or are under-explained. If the commits
are on a branch before merging and you have seen it, all the better, so it can
be fixed before it hits blead. If not, it's something for the author to keep
in mind for next time.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @bulk88

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

Tony

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using
GCC) stop type mismatch warning.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @bulk88

0001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patch
From 272f571b0f05ee9f86e59b3560daf73a5a976e0a Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Thu, 18 Jul 2013 14:55:37 -0400
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v

|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.

Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.
---
 perl.c |   95 ++++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/perl.c b/perl.c
index bad66f5..2c52a6e 100644
--- a/perl.c
+++ b/perl.c
@@ -3502,30 +3502,35 @@ STATIC void
 S_minus_v(pTHX)
 {
 	PerlIO * PIO_stdout;
-	if (!sv_derived_from(PL_patchlevel, "version"))
-	    upg_version(PL_patchlevel, TRUE);
 	{
-	    SV* level= vstringify(PL_patchlevel);
+	    const char * const level_str = "v" PERL_VERSION_STRING;
+	    const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
 #ifdef PERL_PATCHNUM
+	    SV* level;
 #  ifdef PERL_GIT_UNCOMMITTED_CHANGES
-	    SV *num = newSVpvs(PERL_PATCHNUM "*");
+	    static const char num [] = PERL_PATCHNUM "*";
 #  else
-	    SV *num = newSVpvs(PERL_PATCHNUM);
+	    static const char num [] = PERL_PATCHNUM;
 #  endif
 	    {
-		STRLEN level_len, num_len;
-		char * level_str, * num_str;
-		num_str = SvPV(num, num_len);
-		level_str = SvPV(level, level_len);
-		if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
-		    SvREFCNT_dec(level);
-		    level= num;
+		const STRLEN num_len = sizeof(num)-1;
+		/* A very advanced compiler would fold away the strnEQ
+		   and this whole conditional, but most (all?) won't do it.
+		   SV level could also be replaced by with preprocessor
+		   catenation.
+		*/
+		if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+		    /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+		       of the interp so it might contain format characters
+		    */
+		    level = newSVpvn(num, num_len);
 		} else {
-		    Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
-		    SvREFCNT_dec(num);
+		    level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
 		}
 	    }
- #endif
+#else
+	SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
 	PIO_stdout =  PerlIO_stdout();
 	    PerlIO_printf(PIO_stdout,
 		"\nThis is perl "	STRINGIFY(PERL_REVISION)
@@ -3533,59 +3538,61 @@ S_minus_v(pTHX)
 		", subversion "		STRINGIFY(PERL_SUBVERSION)
 		" (%"SVf") built for "	ARCHNAME, level
 		);
-	    SvREFCNT_dec(level);
+	    SvREFCNT_dec_NN(level);
 	}
-#if defined(LOCAL_PATCH_COUNT)
-	if (LOCAL_PATCH_COUNT > 0)
-	    PerlIO_printf(PIO_stdout,
-			  "\n(with %d registered patch%s, "
-			  "see perl -V for more detail)",
-			  LOCAL_PATCH_COUNT,
-			  (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
-
 	PerlIO_printf(PIO_stdout,
-		      "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+    /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+        LOCAL_PATCH_COUNT == 1 ?
+                "\n(with %zu registered patch, see perl -V for more detail)%s"
+        : LOCAL_PATCH_COUNT > 0 ?
+                "\n(with %zu registered patches, see perl -V for more detail)%s"
+        : "%.0s%s",
+        INT2PTR(size_t, LOCAL_PATCH_COUNT),
+#endif
+		      "\n\nCopyright 1987-2013, Larry Wall\n"
 #ifdef MSDOS
-	PerlIO_printf(PIO_stdout,
-		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
 #endif
 #ifdef DJGPP
-	PerlIO_printf(PIO_stdout,
 		      "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
-		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
 #endif
 #ifdef OS2
-	PerlIO_printf(PIO_stdout,
 		      "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
-		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
 #endif
 #ifdef OEMVS
-	PerlIO_printf(PIO_stdout,
-		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
 #endif
 #ifdef __VOS__
-	PerlIO_printf(PIO_stdout,
-		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
 #endif
 #ifdef POSIX_BC
-	PerlIO_printf(PIO_stdout,
-		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+		      "Symbian port by Nokia, 2004-2005\n"
 #endif
+/* UNDER_CE closes the printf call*/
 #ifdef UNDER_CE
-	PerlIO_printf(PIO_stdout,
 			"WINCE port by Rainer Keuchel, 2001-2002\n"
 			"Built on " __DATE__ " " __TIME__ "\n\n");
 	wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
-	PerlIO_printf(PIO_stdout,
-		      "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
 #ifdef BINARY_BUILD_NOTICE
+#  ifndef UNDER_CE
+	); /* close Larry Wall copyright printf */
+#  endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
 	BINARY_BUILD_NOTICE;
 #endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
 	PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
 		      "\n\
 Perl may be copied only under the terms of either the Artistic License or the\n\
 GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @nwc10

On Thu, Jul 18, 2013 at 11​:57​:37AM -0700, bulk88 via RT wrote​:

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

Tony

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using
GCC) stop type mismatch warning.

I'm not sure if everything likes %zu. Particularly older SysV vendor compilers.
Need to check.

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.

I'm pretty sure that mixing #ifdef and pre-processor string concatenation
has upset some compilers in the past. Whilst no particular system is going to
have more than one macro defined​:

#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@​stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@​stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif

the construction looks like it could cause compilers to choke
(So we need to check before changing this)

Also, what's the maximum length from concatenating string literals doing it
this way? IIRC ANSI's minimum limit was something puny like 502 characters.
I don't think that any compiler we encountered was that unforgiving, but
something (IBM's compiler) maxed out at about 2000 characters, which we hit
at times.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @bulk88

On Thu Jul 18 12​:35​:21 2013, nicholas wrote​:

On Thu, Jul 18, 2013 at 11​:57​:37AM -0700, bulk88 via RT wrote​:

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not
using
GCC) stop type mismatch warning.

I'm not sure if everything likes %zu. Particularly older SysV vendor
compilers.
Need to check.

OS C library is irrelevant. The format string is parsed by Perl itself
(Perl_sv_vcatpvfn_flags).

With the copyright lines, use more CPP cating. This reduced the
number of
instructions, and removes a couple alignment null in .rdata image
section.

I'm pretty sure that mixing #ifdef and pre-processor string
concatenation
has upset some compilers in the past. Whilst no particular system is
going to
have more than one macro defined​:

The current preproc conditional code could in theory allow 2 different
OSes to match and both of their strings are included. I think I above
asked whether to #elsif the OS lines but got no comments. In my original
patch I made only 1 OS line to be picked. To reduce controversy, I
reverted it back to the original/current 1 or more OS lines code.

#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@​stratus.com, 1997-
2013\n");
+ "Stratus OpenVOS port by Paul.Green@​stratus.com, 1997-
2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif

the construction looks like it could cause compilers to choke
(So we need to check before changing this)

Also, what's the maximum length from concatenating string literals
doing it
this way? IIRC ANSI's minimum limit was something puny like 502
characters.
I don't think that any compiler we encountered was that unforgiving,
but
something (IBM's compiler) maxed out at about 2000 characters, which
we hit
at times.

Nicholas Clark

larry wall copy right+gpl string came out to 411 bytes+alignment padding
(upto 8 more bytes) in my Win32 binary image using a disassembler.

If CPP token catting doesn't work, I dont think Perl will compile
because of code like

  Perl_croak(aTHX_
  "Can't locate object method \"%"UTF8f
  "\" via package \"%"SVf"\""
  " (perhaps you forgot to load \"%"SVf"\"?)",

ifdefs evaled before string litteral catting,
http​://stackoverflow.com/questions/1476892/poster-with-the-8-phases-of-translation-in-the-c-language

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @tonycoz

On Thu Jul 18 11​:57​:36 2013, bulk88 wrote​:

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

Tony

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using
GCC) stop type mismatch warning.

Thanks for the newSVpvn change, unfortunately the format string error
remains​:

  CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi -W
-Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat
-Wwrite-strings
perl.c​: In function ‘S_minus_v’​:
perl.c​:3600​:9​: warning​: format ‘%s’ expects argument of type ‘char *’,
but argument 3 has type ‘long unsigned int’ [-Wformat]

From clang​:

perl.c​:3555​:9​: warning​: format specifies type 'char *' but the argument
has type
  'size_t' (aka 'unsigned long') [-Wformat]
  INT2PTR(size_t, LOCAL_PATCH_COUNT),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./perl.h​:1630​:26​: note​: expanded from macro 'INT2PTR'
# define INT2PTR(any,d) (any)(d)
  ^~~~~~~~
Note that ANSI C doesn't guarantee that size_t and pointers are the same
size, though I can only think an obsolete platform (Win16 large model)
where they are different sizes.

About the only way I can see to avoid this type of error would be
something like​:

  const char copyrights[] =
  ... #ifdefs for platform messages ...

  LOCAL_PATCH_COUNT == 0
  ? PerlIO_printf(PIO_stdout, larry message "%s", copyrights)
  : LOCAL_PATCH_COUNT == 1
  ? PerlIO_printf(PIO_stdout, larry message "with 1 registered
patch\n%s", copyrights)
  : PerlIO_printf(PIO_stdout, larry message "with %d registered
patches\n%s", LOCAL_PATCH_COUNT, copyrights);

The compiler should optimize away all but one branch, but it's
ridiculous code for the purpose (and doesn't belong in perl.)

This updated patch does nothing about the confusing "where's the end of
the printf" issue which I should probably have been more explicit about
in an earlier message.

(I almost posted about %zu too, since that's not C89, but you're right,
perl itself handles the format.)

WRT your response to Nicholas​:

larry wall copy right+gpl string came out to 411 bytes+alignment
padding (upto 8 more bytes) in my Win32 binary image using a
disassembler.

The C89 limit is 509 characters, and it's a limit on string literals in
general, not concatenation in particular. C99 is more generous allowing
4096 characters.

So, as you said, we're fine here.

If CPP token catting doesn't work, I dont think Perl will compile
because of code like

Perl_croak(aTHX_
"Can't locate object method \"%"UTF8f
"\" via package \"%"SVf"\""
" (perhaps you forgot to load \"%"SVf"\"?)",

ifdefs evaled before string litteral catting,

http​://stackoverflow.com/questions/1476892/poster-with-the-8-phases-of-translation-in-the-c-language

While you're right about the behaviour the C specification requires,
some compilers are unfortunately buggy.

You're also comparing two different code constructs here, Nicholas said
that​:

  "literal1"
#ifdef SOMETHING
  "literal2"
#endif

may cause a compiler to choke (such a compiler is buggy, string literal
concatenation has nothing to do with the preprocessor.)

Your example contains no preprocessor directives, just macro replacements.

I wonder if we should have a document describing such compiler bugs that
we've bothered working around in the past.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @doughera88

On Thu, Jul 18, 2013 at 06​:32​:43PM -0700, "Tony Cook via RT" wrote​:

On Thu Jul 18 11​:57​:36 2013, bulk88 wrote​:

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

Tony

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using
GCC) stop type mismatch warning.

Thanks for the newSVpvn change, unfortunately the format string error
remains​:

      CCCMD =  cc \-DPERL\_CORE \-c \-D\_REENTRANT \-D\_GNU\_SOURCE

-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi -W
-Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat
-Wwrite-strings
perl.c​: In function ‘S_minus_v’​:
perl.c​:3600​:9​: warning​: format ‘%s’ expects argument of type ‘char *’,
but argument 3 has type ‘long unsigned int’ [-Wformat]

From clang​:

perl.c​:3555​:9​: warning​: format specifies type 'char *' but the argument
has type
'size_t' (aka 'unsigned long') [-Wformat]
INT2PTR(size_t, LOCAL_PATCH_COUNT),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./perl.h​:1630​:26​: note​: expanded from macro 'INT2PTR'
# define INT2PTR(any,d) (any)(d)
^~~~~~~~
Note that ANSI C doesn't guarantee that size_t and pointers are the same
size, though I can only think an obsolete platform (Win16 large model)
where they are different sizes.

I confess I didn't dig through the code to figure out why this was a
problem, but in my view, for simply printing out copyright messages,
I don't see the value in fighting portability battles. Just keep it simple.

--
  Andy Dougherty doughera@​lafayette.edu

@p5pRT
Copy link
Author

p5pRT commented Oct 29, 2013

From @bulk88

On Thu Jul 18 18​:32​:42 2013, tonyc wrote​:

On Thu Jul 18 11​:57​:36 2013, bulk88 wrote​:

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

Tony

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not
using
GCC) stop type mismatch warning.

Thanks for the newSVpvn change, unfortunately the format string error
remains​:

CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi
-W
-Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat
-Wwrite-strings
perl.c​: In function ‘S_minus_v’​:
perl.c​:3600​:9​: warning​: format ‘%s’ expects argument of type ‘char *’,
but argument 3 has type ‘long unsigned int’ [-Wformat]

From clang​:

perl.c​:3555​:9​: warning​: format specifies type 'char *' but the
argument
has type
'size_t' (aka 'unsigned long') [-Wformat]
INT2PTR(size_t, LOCAL_PATCH_COUNT),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./perl.h​:1630​:26​: note​: expanded from macro 'INT2PTR'
# define INT2PTR(any,d) (any)(d)
^~~~~~~~
Note that ANSI C doesn't guarantee that size_t and pointers are the
same
size, though I can only think an obsolete platform (Win16 large model)
where they are different sizes.

Patch remade, warning silenced on my clang 3.1 and gcc 4.5.3. I thought of doing a "#pragma clang diagnostic ignored "-Wformat"" also, but it seems unnecessary since clang implemented enough of gcc warnings pragma to work.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Oct 29, 2013

From @bulk88

0001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patch
From ef64b0eddac3937d643ae3f0ec79de8648ec9b72 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Tue, 29 Oct 2013 18:01:09 -0400
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v

|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.

Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.
---
 perl.c | 113 ++++++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 44 deletions(-)

diff --git a/perl.c b/perl.c
index 4c80edb..2dbc9ee 100644
--- a/perl.c
+++ b/perl.c
@@ -3525,35 +3525,47 @@ Perl_moreswitches(pTHX_ const char *s)
     return NULL;
 }
 
-
+#ifdef __GNUC__
+#  if __GNUC__ == 4 && __GNUC_MINOR__ >= 6 || __GNUC__ > 4 /* 4.6 -> */
+#    pragma GCC diagnostic push
+#  endif
+#  if __GNUC__ == 4 && __GNUC_MINOR__ >= 2 || __GNUC__ > 4 /* 4.2 -> */
+#    pragma GCC diagnostic ignored "-Wformat"
+#  endif
+#endif
 STATIC void
 S_minus_v(pTHX)
 {
 	PerlIO * PIO_stdout;
-	if (!sv_derived_from(PL_patchlevel, "version"))
-	    upg_version(PL_patchlevel, TRUE);
 	{
-	    SV* level= vstringify(PL_patchlevel);
+	    const char * const level_str = "v" PERL_VERSION_STRING;
+	    const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
 #ifdef PERL_PATCHNUM
+	    SV* level;
 #  ifdef PERL_GIT_UNCOMMITTED_CHANGES
-	    SV *num = newSVpvs(PERL_PATCHNUM "*");
+	    static const char num [] = PERL_PATCHNUM "*";
 #  else
-	    SV *num = newSVpvs(PERL_PATCHNUM);
+	    static const char num [] = PERL_PATCHNUM;
 #  endif
 	    {
-		STRLEN level_len, num_len;
-		char * level_str, * num_str;
-		num_str = SvPV(num, num_len);
-		level_str = SvPV(level, level_len);
-		if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
-		    SvREFCNT_dec(level);
-		    level= num;
+		const STRLEN num_len = sizeof(num)-1;
+		/* A very advanced compiler would fold away the strnEQ
+		   and this whole conditional, but most (all?) won't do it.
+		   SV level could also be replaced by with preprocessor
+		   catenation.
+		*/
+		if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+		    /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+		       of the interp so it might contain format characters
+		    */
+		    level = newSVpvn(num, num_len);
 		} else {
-		    Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
-		    SvREFCNT_dec(num);
+		    level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
 		}
 	    }
- #endif
+#else
+	SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
 	PIO_stdout =  PerlIO_stdout();
 	    PerlIO_printf(PIO_stdout,
 		"\nThis is perl "	STRINGIFY(PERL_REVISION)
@@ -3561,59 +3573,65 @@ S_minus_v(pTHX)
 		", subversion "		STRINGIFY(PERL_SUBVERSION)
 		" (%"SVf") built for "	ARCHNAME, level
 		);
-	    SvREFCNT_dec(level);
+	    SvREFCNT_dec_NN(level);
 	}
-#if defined(LOCAL_PATCH_COUNT)
-	if (LOCAL_PATCH_COUNT > 0)
-	    PerlIO_printf(PIO_stdout,
-			  "\n(with %d registered patch%s, "
-			  "see perl -V for more detail)",
-			  LOCAL_PATCH_COUNT,
-			  (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
 
+/* gcc pragma above silences "perl.c: In function 'S_minus_v':
+   warning: format '%.0s' expects type 'char *', but argument 3 has type
+   'unsigned int'" below */
 	PerlIO_printf(PIO_stdout,
-		      "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+    /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+        LOCAL_PATCH_COUNT == 1 ?
+                "\n(with %zu registered patch, see perl -V for more detail)%s"
+        : LOCAL_PATCH_COUNT > 0 ?
+                "\n(with %zu registered patches, see perl -V for more detail)%s"
+        : "%.0s%s",
+        INT2PTR(size_t, LOCAL_PATCH_COUNT),
+#endif
+		      "\n\nCopyright 1987-2013, Larry Wall\n"
 #ifdef MSDOS
-	PerlIO_printf(PIO_stdout,
-		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
 #endif
 #ifdef DJGPP
-	PerlIO_printf(PIO_stdout,
 		      "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
-		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
 #endif
 #ifdef OS2
-	PerlIO_printf(PIO_stdout,
 		      "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
-		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
 #endif
 #ifdef OEMVS
-	PerlIO_printf(PIO_stdout,
-		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
 #endif
 #ifdef __VOS__
-	PerlIO_printf(PIO_stdout,
-		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
 #endif
 #ifdef POSIX_BC
-	PerlIO_printf(PIO_stdout,
-		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+		      "Symbian port by Nokia, 2004-2005\n"
 #endif
+/* UNDER_CE closes the printf call*/
 #ifdef UNDER_CE
-	PerlIO_printf(PIO_stdout,
 			"WINCE port by Rainer Keuchel, 2001-2002\n"
 			"Built on " __DATE__ " " __TIME__ "\n\n");
 	wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
-	PerlIO_printf(PIO_stdout,
-		      "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
 #ifdef BINARY_BUILD_NOTICE
+#  ifndef UNDER_CE
+	); /* close Larry Wall copyright printf */
+#  endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
 	BINARY_BUILD_NOTICE;
 #endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
 	PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
 		      "\n\
 Perl may be copied only under the terms of either the Artistic License or the\n\
 GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
@@ -3622,6 +3640,13 @@ this system using \"man perl\" or \"perldoc perl\".  If you have access to the\n
 Internet, point your browser at http://www.perl.org/, the Perl Home Page.\n\n");
 	my_exit(0);
 }
+#ifdef __GNUC__
+#  if __GNUC__ == 4 && __GNUC_MINOR__ >= 6 || __GNUC__ > 4 /* 4.6 -> */
+#    pragma GCC diagnostic pop
+#  elif __GNUC__ == 4 && __GNUC_MINOR__ >= 2 || __GNUC__ > 4 /* 4.2 -> */
+#    pragma GCC diagnostic warning "-Wformat"
+#  endif
+#endif
 
 /* compliments of Tom Christiansen */
 
-- 
1.8.0.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Nov 12, 2013

From @tonycoz

On Tue Oct 29 15​:07​:46 2013, bulk88 wrote​:

On Thu Jul 18 18​:32​:42 2013, tonyc wrote​:

On Thu Jul 18 11​:57​:36 2013, bulk88 wrote​:

On Sun Jul 07 23​:53​:42 2013, tonyc wrote​:

Tony

Patch remade, newSVpvn and %d replaced with %zu to hopefully (not
using
GCC) stop type mismatch warning.

Thanks for the newSVpvn change, unfortunately the format string error
remains​:

CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -ansi
-W
-Wextra -Wdeclaration-after-statement -Wendif-labels -Wc++-compat
-Wwrite-strings
perl.c​: In function ‘S_minus_v’​:
perl.c​:3600​:9​: warning​: format ‘%s’ expects argument of type ‘char
*’,
but argument 3 has type ‘long unsigned int’ [-Wformat]

From clang​:

perl.c​:3555​:9​: warning​: format specifies type 'char *' but the
argument
has type
'size_t' (aka 'unsigned long') [-Wformat]
INT2PTR(size_t, LOCAL_PATCH_COUNT),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./perl.h​:1630​:26​: note​: expanded from macro 'INT2PTR'
# define INT2PTR(any,d) (any)(d)
^~~~~~~~
Note that ANSI C doesn't guarantee that size_t and pointers are the
same
size, though I can only think an obsolete platform (Win16 large
model)
where they are different sizes.

Patch remade, warning silenced on my clang 3.1 and gcc 4.5.3. I
thought of doing a "#pragma clang diagnostic ignored "-Wformat"" also,
but it seems unnecessary since clang implemented enough of gcc
warnings pragma to work.

Silencing the warning does not make the code correct.

It also doesn't fix the painful "where does this statement" end that was introduced in the original patch.

To try and move forward​:

a) I'd love a patch with just the first part of the change - everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest patch.

b) to make progress on the consolidation of the string literals I'd like to suggest the following changes to your approach​:

1) retain the current #if defined(LOCAL_PATCH_COUNT) block, trying to fiddle with that in the way you have been is just too non-C. An optimizing compiler will remove the block when LOCAL_PATCH_COUNT is zero anyway

2) don't try to consolidate the "Perl may be copied ..." printf(), it's the main readability issue with your changes.

3) use a second #ifdef UNDER_CE for the wce_hitreturn rather than trying to close and open the printf around it.

Tony

@p5pRT
Copy link
Author

p5pRT commented Nov 13, 2013

From @bulk88

On Mon Nov 11 16​:53​:38 2013, tonyc wrote​:

To try and move forward​:

a) I'd love a patch with just the first part of the change -
everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest
patch.

1st half split out. Patch attached. I need more time to address the rest of your last post. The title was change, so the next half will be tentatively named "optimize the function " and not "remove the Foo from the function" which is this patch. Also RT # added in body of part 1/this patch.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Nov 13, 2013

From @bulk88

0001-remove-PL_patchlevel-from-S_minus_v.patch
From e8d5cdcfc749ebb9541705427c7eec832f432ace Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Tue, 12 Nov 2013 23:07:16 -0500
Subject: [PATCH] remove PL_patchlevel from S_minus_v

|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.

There should not be any user visible changes to -v with this patch.
switches.t contains a regexp for -v already so no further tests were added.

This patch is part of #116296.
---
 perl.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/perl.c b/perl.c
index 2892a87..e4310cd 100644
--- a/perl.c
+++ b/perl.c
@@ -3443,30 +3443,35 @@ STATIC void
 S_minus_v(pTHX)
 {
 	PerlIO * PIO_stdout;
-	if (!sv_derived_from(PL_patchlevel, "version"))
-	    upg_version(PL_patchlevel, TRUE);
 	{
-	    SV* level= vstringify(PL_patchlevel);
+	    const char * const level_str = "v" PERL_VERSION_STRING;
+	    const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
 #ifdef PERL_PATCHNUM
+	    SV* level;
 #  ifdef PERL_GIT_UNCOMMITTED_CHANGES
-	    SV *num = newSVpvs(PERL_PATCHNUM "*");
+	    static const char num [] = PERL_PATCHNUM "*";
 #  else
-	    SV *num = newSVpvs(PERL_PATCHNUM);
+	    static const char num [] = PERL_PATCHNUM;
 #  endif
 	    {
-		STRLEN level_len, num_len;
-		char * level_str, * num_str;
-		num_str = SvPV(num, num_len);
-		level_str = SvPV(level, level_len);
-		if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
-		    SvREFCNT_dec(level);
-		    level= num;
+		const STRLEN num_len = sizeof(num)-1;
+		/* A very advanced compiler would fold away the strnEQ
+		   and this whole conditional, but most (all?) won't do it.
+		   SV level could also be replaced by with preprocessor
+		   catenation.
+		*/
+		if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+		    /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+		       of the interp so it might contain format characters
+		    */
+		    level = newSVpvn(num, num_len);
 		} else {
-		    Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
-		    SvREFCNT_dec(num);
+		    level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
 		}
 	    }
- #endif
+#else
+	SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
 	PIO_stdout =  PerlIO_stdout();
 	    PerlIO_printf(PIO_stdout,
 		"\nThis is perl "	STRINGIFY(PERL_REVISION)
@@ -3474,7 +3479,7 @@ S_minus_v(pTHX)
 		", subversion "		STRINGIFY(PERL_SUBVERSION)
 		" (%"SVf") built for "	ARCHNAME, level
 		);
-	    SvREFCNT_dec(level);
+	    SvREFCNT_dec_NN(level);
 	}
 #if defined(LOCAL_PATCH_COUNT)
 	if (LOCAL_PATCH_COUNT > 0)
-- 
1.8.0.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Nov 20, 2013

From @tonycoz

On Tue Nov 12 20​:15​:03 2013, bulk88 wrote​:

On Mon Nov 11 16​:53​:38 2013, tonyc wrote​:

To try and move forward​:

a) I'd love a patch with just the first part of the change -
everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest
patch.

1st half split out. Patch attached. I need more time to address the
rest of your last post. The title was change, so the next half will be
tentatively named "optimize the function " and not "remove the Foo
from the function" which is this patch. Also RT # added in body of
part 1/this patch.

Thanks, applied as 709aee9.

Tony

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

From @bulk88

On Mon Nov 11 16​:53​:38 2013, tonyc wrote​:

Silencing the warning does not make the code correct.

CC warning free now, is it a ptr or is it an int in the format string design is gone.

It also doesn't fix the painful "where does this statement" end that
was introduced in the original patch.

.......

2) don't try to consolidate the "Perl may be copied ..." printf(),
it's the main readability issue with your changes.

I changed the comments to be clearer where the concatenation goes.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

From @bulk88

0001-optimize-S_minus_v.patch
From 407983156b9b3f0f58e179b653d900df7adb3b08 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sun, 22 Dec 2013 12:19:34 -0500
Subject: [PATCH] optimize S_minus_v

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line. LOCAL_PATCH_COUNT is a C expression,
like offsetof, which is not a preprocessor constant, so add
PERL_GIT_UNPUSHED_COMMITS_LEN and LOCAL_PATCH_COUNT_CONST which are pure
CPP constants. LOCAL_PATCH_COUNT can be changed to LOCAL_PATCH_COUNT_CONST
in the future.

Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.

This patch is part of #116296.
---
 make_patchnum.pl |    9 ++++---
 patchlevel.h     |   10 +++++++++
 perl.c           |   60 ++++++++++++++++++++++++++++++-----------------------
 3 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/make_patchnum.pl b/make_patchnum.pl
index 3f857b5..82ec266 100644
--- a/make_patchnum.pl
+++ b/make_patchnum.pl
@@ -122,6 +122,7 @@ sub write_files {
     return 0;
 }
 
+my @unpushed_commits;
 my $unpushed_commits = '    ';
 my ($read, $branch, $snapshot_created, $commit_id, $describe)= ("") x 5;
 my ($changed, $extra_info, $commit_title)= ("") x 3;
@@ -155,14 +156,13 @@ elsif (-d "$srcdir/.git") {
     }
 
     if (length $branch && length $remote) {
+        @unpushed_commits = grep {/\+/} backtick("git cherry $remote/$merge");
         # git cherry $remote/$branch | awk 'BEGIN{ORS=","} /\+/ {print $2}' | sed -e 's/,$//'
         my $unpushed_commit_list =
-            join ",", map { (split /\s/, $_)[1] }
-            grep {/\+/} backtick("git cherry $remote/$merge");
+            join ",", map { (split /\s/, $_)[1] } @unpushed_commits;
         # git cherry $remote/$branch | awk 'BEGIN{ORS="\t\\\\\n"} /\+/ {print ",\"" $2 "\""}'
         $unpushed_commits =
-            join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" }
-            grep {/\+/} backtick("git cherry $remote/$merge");
+            join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } @unpushed_commits;
         if (length $unpushed_commits) {
             $commit_title = "Local Commit:";
             my $ancestor = backtick("git rev-parse $remote/$merge");
@@ -185,6 +185,7 @@ write_files(<<"EOF_HEADER", <<"EOF_CONFIG");
 *          DO NOT EDIT DIRECTLY - edit make_patchnum.pl instead
 ***************************************************************************/
 @{[$describe ? "#define PERL_PATCHNUM \"$describe\"" : ()]}
+#define PERL_GIT_UNPUSHED_COMMITS_LEN ${\scalar(@unpushed_commits)}
 #define PERL_GIT_UNPUSHED_COMMITS\t\t\\
 $unpushed_commits/*leave-this-comment*/
 @{[$changed ? "#define PERL_GIT_UNCOMMITTED_CHANGES" : ()]}
diff --git a/patchlevel.h b/patchlevel.h
index a7e8dac..cd60a71 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -125,9 +125,11 @@ hunk.
 #  if defined(PERL_IS_MINIPERL)
 #    define PERL_PATCHNUM "UNKNOWN-miniperl"
 #    define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+#    define PERL_GIT_UNPUSHED_COMMITS_LEN 0
 #  elif defined(PERL_MICRO)
 #    define PERL_PATCHNUM "UNKNOWN-microperl"
 #    define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+#    define PERL_GIT_UNPUSHED_COMMITS_LEN 0
 #  else
 #include "git_version.h"
 #  endif
@@ -146,6 +148,14 @@ static const char * const local_patches[] = {
 #  define	LOCAL_PATCH_COUNT	\
 	((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
 
+#ifdef PERL_GIT_UNCOMMITTED_CHANGES
+#  define	LOCAL_PATCH_COUNT_CONST	\
+    (PERL_GIT_UNPUSHED_COMMITS_LEN+1)
+#else
+#  define	LOCAL_PATCH_COUNT_CONST	\
+    (PERL_GIT_UNPUSHED_COMMITS_LEN)
+#endif
+
 /* the old terms of reference, add them only when explicitly included */
 #define PATCHLEVEL		PERL_VERSION
 #undef  SUBVERSION		/* OS/390 has a SUBVERSION in a system header */
diff --git a/perl.c b/perl.c
index 8271915..ad7db6a 100644
--- a/perl.c
+++ b/perl.c
@@ -3480,56 +3480,64 @@ S_minus_v(pTHX)
 	    SvREFCNT_dec_NN(level);
 	}
 #if defined(LOCAL_PATCH_COUNT)
-	if (LOCAL_PATCH_COUNT > 0)
-	    PerlIO_printf(PIO_stdout,
-			  "\n(with %d registered patch%s, "
-			  "see perl -V for more detail)",
-			  LOCAL_PATCH_COUNT,
-			  (LOCAL_PATCH_COUNT!=1) ? "es" : "");
+        assert(LOCAL_PATCH_COUNT_CONST == LOCAL_PATCH_COUNT);
 #endif
-
+/* start of Larry Wall copyright printf */
 	PerlIO_printf(PIO_stdout,
-		      "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT) && LOCAL_PATCH_COUNT_CONST != 0
+    /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+#  if LOCAL_PATCH_COUNT_CONST == 1
+	    "\n(with %d registered patch, see perl -V for more detail)%s"
+#  else
+	    "\n(with %d registered patches, see perl -V for more detail)%s"
+#  endif
+	    ,LOCAL_PATCH_COUNT_CONST,
+#else
+    /* use copyrights for format string */
+#endif
+		      "\n\nCopyright 1987-2013, Larry Wall\n"
 #ifdef MSDOS
-	PerlIO_printf(PIO_stdout,
-		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
 #endif
 #ifdef DJGPP
-	PerlIO_printf(PIO_stdout,
 		      "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
-		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
 #endif
 #ifdef OS2
-	PerlIO_printf(PIO_stdout,
 		      "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
-		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
 #endif
 #ifdef OEMVS
-	PerlIO_printf(PIO_stdout,
-		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
 #endif
 #ifdef __VOS__
-	PerlIO_printf(PIO_stdout,
-		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
 #endif
 #ifdef POSIX_BC
-	PerlIO_printf(PIO_stdout,
-		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+		      "Symbian port by Nokia, 2004-2005\n"
 #endif
+/* UNDER_CE closes Larry Wall copyright printf */
 #ifdef UNDER_CE
-	PerlIO_printf(PIO_stdout,
 			"WINCE port by Rainer Keuchel, 2001-2002\n"
 			"Built on " __DATE__ " " __TIME__ "\n\n");
 	wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
-	PerlIO_printf(PIO_stdout,
-		      "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
 #ifdef BINARY_BUILD_NOTICE
+#  ifndef UNDER_CE
+	); /* close Larry Wall copyright printf */
+#  endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
 	BINARY_BUILD_NOTICE;
 #endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
 	PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
 		      "\n\
 Perl may be copied only under the terms of either the Artistic License or the\n\
 GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Mar 5, 2014

From @bulk88

On Sun Dec 22 09​:23​:16 2013, bulk88 wrote​:

On Mon Nov 11 16​:53​:38 2013, tonyc wrote​:

Silencing the warning does not make the code correct.

CC warning free now, is it a ptr or is it an int in the format string
design is gone.

It also doesn't fix the painful "where does this statement" end that
was introduced in the original patch.

.......

2) don't try to consolidate the "Perl may be copied ..." printf(),
it's the main readability issue with your changes.

I changed the comments to be clearer where the concatenation goes.
Bump. Patch needs review and applying.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 8, 2014

From @tonycoz

On Wed Mar 05 04​:49​:43 2014, bulk88 wrote​:

Bump. Patch needs review and applying.

This fails to apply to blead, I haven't tried to apply it manually.

Tony

@p5pRT
Copy link
Author

p5pRT commented Mar 8, 2014

From @bulk88

On Fri Mar 07 19​:00​:30 2014, tonyc wrote​:

On Wed Mar 05 04​:49​:43 2014, bulk88 wrote​:

Bump. Patch needs review and applying.

This fails to apply to blead, I haven't tried to apply it manually.

Tony

I fixed the conflicts and created a new patch. Patch is testing right now on my system.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 8, 2014

From @bulk88

On Fri Mar 07 20​:09​:09 2014, bulk88 wrote​:

On Fri Mar 07 19​:00​:30 2014, tonyc wrote​:

On Wed Mar 05 04​:49​:43 2014, bulk88 wrote​:

Bump. Patch needs review and applying.

This fails to apply to blead, I haven't tried to apply it manually.

Tony

I fixed the conflicts and created a new patch. Patch is testing right
now on my system.
Will attach when that is done.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 8, 2014

From @bulk88

On Fri Mar 07 20​:09​:36 2014, bulk88 wrote​:

Will attach when that is done.

New patch against blead attached.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 8, 2014

From @bulk88

0001-optimize-S_minus_v.patch
From b5787d4e10533e116b931570f821bb7254fe2721 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sun, 22 Dec 2013 12:19:34 -0500
Subject: [PATCH] optimize S_minus_v

With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line. LOCAL_PATCH_COUNT is a C expression,
like offsetof, which is not a preprocessor constant, so add
PERL_GIT_UNPUSHED_COMMITS_LEN and LOCAL_PATCH_COUNT_CONST which are pure
CPP constants. LOCAL_PATCH_COUNT can be changed to LOCAL_PATCH_COUNT_CONST
in the future.

Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.

This patch is part of #116296.
---
 make_patchnum.pl |    9 ++++---
 patchlevel.h     |   10 +++++++++
 perl.c           |   60 ++++++++++++++++++++++++++++++-----------------------
 3 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/make_patchnum.pl b/make_patchnum.pl
index 3f857b5..82ec266 100644
--- a/make_patchnum.pl
+++ b/make_patchnum.pl
@@ -122,6 +122,7 @@ sub write_files {
     return 0;
 }
 
+my @unpushed_commits;
 my $unpushed_commits = '    ';
 my ($read, $branch, $snapshot_created, $commit_id, $describe)= ("") x 5;
 my ($changed, $extra_info, $commit_title)= ("") x 3;
@@ -155,14 +156,13 @@ elsif (-d "$srcdir/.git") {
     }
 
     if (length $branch && length $remote) {
+        @unpushed_commits = grep {/\+/} backtick("git cherry $remote/$merge");
         # git cherry $remote/$branch | awk 'BEGIN{ORS=","} /\+/ {print $2}' | sed -e 's/,$//'
         my $unpushed_commit_list =
-            join ",", map { (split /\s/, $_)[1] }
-            grep {/\+/} backtick("git cherry $remote/$merge");
+            join ",", map { (split /\s/, $_)[1] } @unpushed_commits;
         # git cherry $remote/$branch | awk 'BEGIN{ORS="\t\\\\\n"} /\+/ {print ",\"" $2 "\""}'
         $unpushed_commits =
-            join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" }
-            grep {/\+/} backtick("git cherry $remote/$merge");
+            join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } @unpushed_commits;
         if (length $unpushed_commits) {
             $commit_title = "Local Commit:";
             my $ancestor = backtick("git rev-parse $remote/$merge");
@@ -185,6 +185,7 @@ write_files(<<"EOF_HEADER", <<"EOF_CONFIG");
 *          DO NOT EDIT DIRECTLY - edit make_patchnum.pl instead
 ***************************************************************************/
 @{[$describe ? "#define PERL_PATCHNUM \"$describe\"" : ()]}
+#define PERL_GIT_UNPUSHED_COMMITS_LEN ${\scalar(@unpushed_commits)}
 #define PERL_GIT_UNPUSHED_COMMITS\t\t\\
 $unpushed_commits/*leave-this-comment*/
 @{[$changed ? "#define PERL_GIT_UNCOMMITTED_CHANGES" : ()]}
diff --git a/patchlevel.h b/patchlevel.h
index 37dda6a..39191fb 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -125,9 +125,11 @@ hunk.
 #  if defined(PERL_IS_MINIPERL)
 #    define PERL_PATCHNUM "UNKNOWN-miniperl"
 #    define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+#    define PERL_GIT_UNPUSHED_COMMITS_LEN 0
 #  elif defined(PERL_MICRO)
 #    define PERL_PATCHNUM "UNKNOWN-microperl"
 #    define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+#    define PERL_GIT_UNPUSHED_COMMITS_LEN 0
 #  else
 #include "git_version.h"
 #  endif
@@ -146,6 +148,14 @@ static const char * const local_patches[] = {
 #  define	LOCAL_PATCH_COUNT	\
 	((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
 
+#ifdef PERL_GIT_UNCOMMITTED_CHANGES
+#  define	LOCAL_PATCH_COUNT_CONST	\
+    (PERL_GIT_UNPUSHED_COMMITS_LEN+1)
+#else
+#  define	LOCAL_PATCH_COUNT_CONST	\
+    (PERL_GIT_UNPUSHED_COMMITS_LEN)
+#endif
+
 /* the old terms of reference, add them only when explicitly included */
 #define PATCHLEVEL		PERL_VERSION
 #undef  SUBVERSION		/* OS/390 has a SUBVERSION in a system header */
diff --git a/perl.c b/perl.c
index 36d33d9..cb7d62a 100644
--- a/perl.c
+++ b/perl.c
@@ -3498,56 +3498,64 @@ S_minus_v(pTHX)
 	    SvREFCNT_dec_NN(level);
 	}
 #if defined(LOCAL_PATCH_COUNT)
-	if (LOCAL_PATCH_COUNT > 0)
-	    PerlIO_printf(PIO_stdout,
-			  "\n(with %d registered patch%s, "
-			  "see perl -V for more detail)",
-			  LOCAL_PATCH_COUNT,
-			  (LOCAL_PATCH_COUNT!=1) ? "es" : "");
+        assert(LOCAL_PATCH_COUNT_CONST == LOCAL_PATCH_COUNT);
 #endif
-
+/* start of Larry Wall copyright printf */
 	PerlIO_printf(PIO_stdout,
-		      "\n\nCopyright 1987-2014, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT) && LOCAL_PATCH_COUNT_CONST != 0
+    /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+#  if LOCAL_PATCH_COUNT_CONST == 1
+	    "\n(with %d registered patch, see perl -V for more detail)%s"
+#  else
+	    "\n(with %d registered patches, see perl -V for more detail)%s"
+#  endif
+	    ,LOCAL_PATCH_COUNT_CONST,
+#else
+    /* use copyrights for format string */
+#endif
+		      "\n\nCopyright 1987-2014, Larry Wall\n"
 #ifdef MSDOS
-	PerlIO_printf(PIO_stdout,
-		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+		      "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
 #endif
 #ifdef DJGPP
-	PerlIO_printf(PIO_stdout,
 		      "djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
-		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+		      "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
 #endif
 #ifdef OS2
-	PerlIO_printf(PIO_stdout,
 		      "\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
-		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+		      "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
 #endif
 #ifdef OEMVS
-	PerlIO_printf(PIO_stdout,
-		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+		      "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
 #endif
 #ifdef __VOS__
-	PerlIO_printf(PIO_stdout,
-		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+		      "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
 #endif
 #ifdef POSIX_BC
-	PerlIO_printf(PIO_stdout,
-		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+		      "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+		      "Symbian port by Nokia, 2004-2005\n"
 #endif
+/* UNDER_CE closes Larry Wall copyright printf */
 #ifdef UNDER_CE
-	PerlIO_printf(PIO_stdout,
 			"WINCE port by Rainer Keuchel, 2001-2002\n"
 			"Built on " __DATE__ " " __TIME__ "\n\n");
 	wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
-	PerlIO_printf(PIO_stdout,
-		      "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
 #ifdef BINARY_BUILD_NOTICE
+#  ifndef UNDER_CE
+	); /* close Larry Wall copyright printf */
+#  endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
 	BINARY_BUILD_NOTICE;
 #endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
 	PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
 		      "\n\
 Perl may be copied only under the terms of either the Artistic License or the\n\
 GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented Mar 19, 2014

From @bulk88

On Fri Mar 07 22​:02​:52 2014, bulk88 wrote​:

On Fri Mar 07 20​:09​:36 2014, bulk88 wrote​:

Will attach when that is done.

New patch against blead attached.

Bump.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 24, 2014

From @rjbs

While I'd like to see this ticket reviewed, I am declining to mark it as a blocker for 5.20.0.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Apr 4, 2014

From @bulk88

On Mon Mar 24 07​:16​:06 2014, rjbs wrote​:

While I'd like to see this ticket reviewed, I am declining to mark it
as a blocker for 5.20.0.

Bump.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2014

From @tonycoz

On Thu Apr 03 23​:23​:16 2014, bulk88 wrote​:

On Mon Mar 24 07​:16​:06 2014, rjbs wrote​:

While I'd like to see this ticket reviewed, I am declining to mark it
as a blocker for 5.20.0.

Bump.

I won't accept it while that printf() can end in more than one place.

Right now it can end in three different places​:

#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
  "WINCE port by Rainer Keuchel, 2001-2002\n"
  "Built on " __DATE__ " " __TIME__ "\n\n"); **HERE
  wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */ **HERE
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
  BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
  PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
  "\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
** SOMEWHERE BELOW HERE

Unfortunately I expect that will remove the rather trivial space improvement the patch brings.

Otherwise, I'm worried about the issue Nicholas mentioned, where some compilers might choke on the mix of pre-processor directives and string literal concatenation.

Tony

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants