Dual-Life / Devel-PPPort

Perl/Pollution/Portability
9 stars 28 forks source link

Fix sv_2pv and sv_2pv_flags for Perl < 5.17.2 #232

Closed pali closed 1 year ago

pali commented 1 year ago

Fixes: https://github.com/Dual-Life/Devel-PPPort/issues/231

pali commented 1 year ago

Can anybody look at this change? @pmqs @atoomic @khwilliamson

pmqs commented 1 year ago

Seeing a compilation warning with this change

In file included from Zlib.xs:216:
ppport.h:14501:7: warning: extra tokens at end of #else directive [-Wendif-labels]
14501 | #else if (PERL_BCDVERSION < 0x5017002)
      |       ^~

assume you meant to use #elif?

#elif (PERL_BCDVERSION < 0x5017002)
pmqs commented 1 year ago

Apart from the #else if warning all is good at my end. The Perl 5.6.0 build of Compress-Raw-Zlib worked fine.

pali commented 1 year ago

I force pushed fix with elif. Please check.

pmqs commented 1 year ago

I force pushed fix with elif. Please check.

All is fine here with the elif change

Also happened to notice this warning when building Devel-PPPort - don't think that is anything to do with this change though. Can create a new issue if needed.

gcc-13 -c   -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2   -DVERSION=\"3.68\" -DXS_VERSION=\"3.68\" -fPIC "-I/home/paul/base/install/gcc-13/perl/std/5.38.0/lib/5.38.0/x86_64-linux/CORE"  -W -Wall module3.c
module3.c: In function ‘dummy_parser_warning’:
module3.c:70:21: warning: the comparison will always evaluate as ‘true’ for the address of ‘bufptr’ will never be NULL [-Waddress]
   70 |   return &PL_bufptr != NULL;
      |                     ^~
In file included from /home/paul/base/install/gcc-13/perl/std/5.38.0/lib/5.38.0/x86_64-linux/CORE/perl.h:4556,
                 from module3.c:17:
/home/paul/base/install/gcc-13/perl/std/5.38.0/lib/5.38.0/x86_64-linux/CORE/parser.h:83:18: note: ‘bufptr’ declared here
   83 |     char        *bufptr;        /* carries the cursor (current parsing
      |                  ^~~~~~
atoomic commented 1 year ago

I've submitted the changes to blead via https://github.com/Perl/perl5/pull/21453

khwilliamson commented 1 year ago

I started a review of this and got distracted, and the comments I had started did not get into the record.

I do want to review this thoroughly. One issue is the use of PL_na. Note the BEWARE clause in perlapi.

"PL_na" A scratch pad variable in which to store a "STRLEN" value. If would have been better named something like "PL_temp_strlen".

     It is is typically used with "SvPV" when one is actually planning to
     discard the returned length, (hence the length is "Not Applicable",
     which is how this variable got its name).

     BUT BEWARE, if this is used in a situation where something that is
     using it is in a call stack with something else that is using it,
     this variable would get zapped, leading to hard-to-diagnose errors.

     It is usually more efficient to either declare a local variable and
     use that instead, or to use the "SvPV_nolen" macro.

Sometimes one has to use PL_na, but it should not be used when we have GCC brace groups available

pali commented 1 year ago

Makes sense.