p5pRT
commented
11 years ago From @bulk88
See attached patch.
Open p5pRT opened 11 years ago
p5pRT
commented
11 years ago See attached patch.
p5pRT
commented
11 years ago p5pRT
commented
11 years ago 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
commented
11 years ago The RT System itself - Status changed from 'new' to 'open'
p5pRT
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago p5pRT
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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
p5pRT
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago * 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
commented
11 years ago 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
commented
11 years ago p5pRT
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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
commented
10 years ago 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
commented
10 years ago p5pRT
commented
10 years ago 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
commented
10 years ago 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
commented
10 years ago p5pRT
commented
10 years ago 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 709aee9447f22f3f59d548d44298cca98a023f80.
Tony
p5pRT
commented
10 years ago 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
commented
10 years ago p5pRT
commented
10 years ago 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
commented
10 years ago 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
commented
10 years ago 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
commented
10 years ago 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
commented
10 years ago 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
commented
10 years ago p5pRT
commented
10 years ago 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
commented
10 years ago While I'd like to see this ticket reviewed\, I am declining to mark it as a blocker for 5.20.0.
-- rjbs
p5pRT
commented
10 years ago 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
commented
10 years ago 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
Migrated from rt.perl.org#116296 (status was 'open')
Searchable as RT116296$