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

Fix sv_2pv and sv_2pv_flags for Perl < 5.17.2 #232

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

pali
Copy link
Contributor

@pali pali commented Aug 23, 2023

Fixes: #231

@pali
Copy link
Contributor Author

pali commented Sep 3, 2023

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

Copy link
Member

@atoomic atoomic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pmqs
Copy link

pmqs commented Sep 3, 2023

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
Copy link

pmqs commented Sep 3, 2023

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
Copy link
Contributor Author

pali commented Sep 3, 2023

I force pushed fix with elif. Please check.

@pmqs
Copy link

pmqs commented Sep 3, 2023

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 atoomic merged commit d4ef817 into Dual-Life:master Sep 4, 2023
18 of 21 checks passed
@pali pali deleted the sv_2pv branch September 4, 2023 22:24
@atoomic
Copy link
Member

atoomic commented Sep 4, 2023

I've submitted the changes to blead via Perl/perl5#21453

@khwilliamson
Copy link
Member

khwilliamson commented Sep 5, 2023

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
Copy link
Contributor Author

pali commented Sep 11, 2023

Makes sense.

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

Successfully merging this pull request may close these issues.

Regression with Perl 5.6.0 in the Compress::Raw::* modules
4 participants