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

Implement SvIV_nomg(), SvUV_nomg(), SvNV_nomg() and SvTRUE_nomg() #128

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

pali
Copy link
Contributor

@pali pali commented Jul 12, 2019

Use sv_mortalcopy_flags() macro with SV_NOSTEAL flag to create non-magical
copy of input scalar. And on this non-magical copy call original Perl's
SvIV/SvUV/SvNV/SvTRUE macro.

This would ensure that get magic is not processed on original input scalar
argument and also that correct value is returned.

Use sv_mortalcopy_flags() macro with SV_NOSTEAL flag to create non-magical
copy of input scalar. And on this non-magical copy call original Perl's
SvIV/SvUV/SvNV/SvTRUE macro.

This would ensure that get magic is not processed on original input scalar
argument and also that correct value is returned.
tie my $scalar, 'TieScalarCounter', 10;
my $fetch = $scalar;

ok tied($scalar)->{fetch}, 1;
Copy link
Member

Choose a reason for hiding this comment

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

should not these tests use is instead of ok?

Copy link
Contributor Author

@pali pali Jul 12, 2019

Choose a reason for hiding this comment

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

Devel::PPPort does not use Test::Simple, but rather Test module which has different API. See:
https://github.com/Dual-Life/Devel-PPPort/pull/128/files#diff-e998a77b1ca4b0163f81709c60da878fR29
And it has only 3 argument ok function, see:
https://github.com/Dual-Life/Devel-PPPort/blob/master/t/testutil.pl#L20
It does not support is function.

Copy link
Member

Choose a reason for hiding this comment

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

This is in order to maintain compatibility with perls < 5.6. However, we could write our own is, isnt, like functions in a local library (wrapping Test::ok) that provide the same APIs as Test::More's functions, to make it easier to write (and read) the tests.

Copy link
Member

Choose a reason for hiding this comment

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

This is not the first time that I haven't received a notification from github. How can I track the cause of this down. Is it github, is it cpan.org?.

Having stumbled upon this, I agree with ether's sentiment. Who is going to write this wrapper?

Copy link
Member

Choose a reason for hiding this comment

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

notifications are coming from GitHub itself, you should check your subscription status to the repo but also to the issue itself

Copy link
Member

Choose a reason for hiding this comment

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

I've opened #130 for tracking this wish list, I would say the first that want to work on it should assign the task to himself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, using new test wrapper would mean to rewrite all tests... I'm not planning to do it as it is a big work right now and we can leave without it.

@@ -118,3 +118,68 @@ my $foo = 'bar';
ok(Devel::PPPort::sv_magic_portable($foo));
ok($foo eq 'bar');

if ( "$]" lt '5.007003' ) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be < instead of lt, because of bugs in early perls, which I have discovered in testing them

Copy link
Member

Choose a reason for hiding this comment

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

yeah agree then the '5.007003' could simply become 5.007003

Copy link
Member

Choose a reason for hiding this comment

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

note that "$]" < is required to workaround that bug with older perl versions

Copy link
Member

Choose a reason for hiding this comment

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

view #131

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that this pattern needs to be fixed on more places in Devel::PPPort package.

@pali
Copy link
Contributor Author

pali commented Jul 17, 2019

Is there anything else which needs to be fixed in this pull request? I guess that both changes would be done globally for all Devel::PPPort code.

@khwilliamson
Copy link
Member

khwilliamson commented Jul 17, 2019 via email

@khwilliamson khwilliamson merged commit 8173a75 into Dual-Life:master Jul 17, 2019
@pali pali deleted the nomg branch July 18, 2019 07:50
@khwilliamson
Copy link
Member

can this issue be closed?

@atoomic
Copy link
Member

atoomic commented Oct 14, 2019

@khwilliamson this PR #128 is merged and thus do not appear as open, we cannot close it, its status is merged.

Are you asking to close another issue? could you be more specific about the number?

thanks

@khwilliamson
Copy link
Member

khwilliamson commented Oct 14, 2019 via email

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.

4 participants