Skip to content

Polyfill for __SUB__ token - #46

Open
jberger wants to merge 3 commits into
Dual-Life:masterfrom
jberger:current_sub
Open

Polyfill for __SUB__ token#46
jberger wants to merge 3 commits into
Dual-Life:masterfrom
jberger:current_sub

Conversation

@jberger

@jberger jberger commented Jun 6, 2016

Copy link
Copy Markdown

Per the discussion on https://rt.cpan.org/Public/Bug/Display.html?id=111466 this PR implements a polyfill for the __SUB__ token. The code is mostly lifted from Sub::Current (Rafael Garcia-Suarez). I did the porting and Aaron Crane helped with an issue I ran into.

The one remaining question in my mind is if ROUTINE should be exported/exportable or not. The case for it is that it would allow a cautious user to always use the polyfill implementation rather than relying on __SUB__ being the polyfill on old perl versions vs being the core token in newer versions. The case against is the added complexity of having two different names doing the ostensibly the same thing. Thoughts?

The difference between __SUB__ and Sub::Current::ROUTINE is the behavior inside the BEGIN/END/etc blocks. As a result this version of ROUTINE differs slightly from Sub::Current to match the behavior of __SUB__.

Comment thread lib/Sub/Util.pm Outdated
require List::Util; # as it has the XS
List::Util->VERSION( $VERSION ); # Ensure we got the right XS version (RT#100863)

use constant CAN_SUB => $^V >= 5.016;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should check against $], not $^V.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

$^V is not available in 5.6 for example and stands for v5.x in later version
even if the compare is not incorrect for modern Perl version
you should use $]

@jberger

jberger commented Jun 6, 2016

Copy link
Copy Markdown
Author

as discussed on IRC this PR should probably be held until ROUTINE is made private (I think that is the side we fell to) and some documentation is added.

@atoomic atoomic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

view comment about $] instead of $^V.

@jberger

jberger commented Aug 11, 2020

Copy link
Copy Markdown
Author

Is it as simple as changing the variable? Do I also have to change the comparison?

@Grinnz

Grinnz commented Aug 11, 2020

Copy link
Copy Markdown
Contributor

No, it is already comparing against the right decimal number.

@jberger

jberger commented Aug 11, 2020

Copy link
Copy Markdown
Author

This branch is very stale but locally a rebased version passes tests. Any opposition to me force-pushing the rebased version?

@atoomic

atoomic commented Aug 11, 2020

Copy link
Copy Markdown
Member

I would say rebase && force push. The CI will confirm how it stands with the various Perl version
Note: we are currently using Travis CI, but I've provided a quick and fast alternate addition using GH CI as #110

@jberger

jberger commented Aug 12, 2020

Copy link
Copy Markdown
Author

OK so it fails on 5.6. Is that fixable? Is it allowable?

@atoomic

atoomic commented Aug 12, 2020

Copy link
Copy Markdown
Member

@jberger the problem comes from Perl_find_runcv, when running ppport.h
we can see that this is not portable before 5.8.1

*** WARNING: Uses find_runcv, which may not be portable below perl 5.8.1, even with 'ppport.h'

So I came with a poor man alternate with an additional commit to provide it when missing, and it seems to work...
the alternate is to drop the support below 5.6

view commit:
atoomic@c74c017

view CI proof of concept:
https://github.com/atoomic/Scalar-List-Utils/actions/runs/205763453

So cherry picking this commit to your branch should fix your concern.

@jberger

jberger commented Aug 12, 2020

Copy link
Copy Markdown
Author

@jberger the problem comes from Perl_find_runcv, when running ppport.h
we can see that this is not portable before 5.8.1
[SNIP]
So I came with a poor man alternate with an additional commit to provide it when missing, and it seems to work...
the alternate is to drop the support below 5.6
[SNIP]
So cherry picking this commit to your branch should fix your concern.

That's beyond my power to evaluate. @leonerd thoughts?

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.

5 participants