Skip to content

perlapi: Document sv_dup(_inc)? #19741

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

Merged
merged 1 commit into from
May 20, 2022
Merged

Conversation

khwilliamson
Copy link
Contributor

No description provided.

@Leont
Copy link
Contributor

Leont commented May 18, 2022

That first paragraph probably needs an additional sentence along the lines of The latter is almost always what you want., but I'm not sure how to explain why. Basically you should used the _inc variant if the source location owned a refcount on the SV.

@khwilliamson khwilliamson merged commit f45ba10 into Perl:blead May 20, 2022
@iabyn
Copy link
Contributor

iabyn commented May 21, 2022 via email

@Leont
Copy link
Contributor

Leont commented May 21, 2022

The new pod for this is making my skin tingle.

sv_dup() is a highly specialised function for cloning an arbitrary SV into a new interpreter - principally used during ithreads thread creation.

The new POD for this function makes it sound instead like it's a general-purpose SV-copying function.

I think a lot of the recently added documentation is focused on what a function does, not why you'd want to use it. It's the common order of documenting things, but in this case that works out less than ideally.

I'm not sure in principle whether this function should even be in the API.

It should IMO. They are used on CPAN to clone data, and there's literally no other way to do such a thing. The main issue IME is that the CLONE_PARAMS isn't available everyhere where one may want to use this

@khwilliamson
Copy link
Contributor Author

khwilliamson commented May 23, 2022

This patch was reverted due to the contention at the deadline. improvement suggestions welcome!

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

Successfully merging this pull request may close these issues.

3 participants