develooper Front page | perl.perl5.porters | Postings from July 2013

Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimizeS_minus_v

Thread Previous | Thread Next
From:
Tony Cook
Date:
July 8, 2013 06:53
Subject:
Re: [perl #116296] [PATCH] remove PL_patchlevel from and optimizeS_minus_v
Message ID:
20130708065252.GC12268@mars.tony.develop-help.com
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 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 */

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

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About