Skip to content

Add builtin::export_lexically #19895

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 15, 2022
Merged

Add builtin::export_lexically #19895

merged 3 commits into from
Jul 15, 2022

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Jun 27, 2022

Adds a builtin:: function for performing the lexical-export behaviour, so that new code can do the same trickery that core builtin:: itself achieves.

@leonerd leonerd added the squash-before-merge Author must squash the commits down before merging to blead label Jun 27, 2022
@leonerd leonerd mentioned this pull request Jun 27, 2022
Open
ilmari
ilmari requested changes Jun 27, 2022
View reviewed changes
@leonerd leonerd force-pushed the lexically-export branch from 14ccf16 to 7978e66 Compare June 27, 2022 16:09
@leonerd
Copy link
Contributor Author

leonerd commented Jun 27, 2022

Thanks @ilmari. Comments addressed and force-pushed.

ilmari
ilmari approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@ilmari ilmari left a comment

Choose a reason for hiding this comment

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

LGTM

@leonerd leonerd force-pushed the lexically-export branch from 7978e66 to 0736d65 Compare June 27, 2022 20:06
@leonerd leonerd removed the squash-before-merge Author must squash the commits down before merging to blead label Jun 27, 2022
@tonycoz
Copy link
Contributor

tonycoz commented Jun 28, 2022

#19857 is still a problem here:

tony@venus:.../git/perl5$ ./perl -Ilib -Mbuiltin=lexically_export -e 'BEGIN { lexically_export abc => sub {}; } BEGIN { eval "abc()" }'
Built-in function 'builtin::lexically_export' is experimental at -e line 1.
perl: op.c:13404: Perl_find_lexical_cv: Assertion `PARENT_PAD_INDEX(name)' failed.
Aborted

@ilmari
Copy link
Member

ilmari commented Jun 28, 2022

#19857 is still a problem here:

Seeing as that's a problem with lexical subs in general, not specifically with exporting them, I don't think that should block this.

@leonerd
Copy link
Contributor Author

leonerd commented Jun 28, 2022

P5P requests that this be expanded into a full RFC. I am now in the process of writing that. Until that's agreed, we should not merge this (but it can be referenced as a prototype implementation)

@leonerd
Copy link
Contributor Author

leonerd commented Jun 28, 2022

RFC now written and PR raised: Perl/PPCs#20

@leonerd leonerd force-pushed the lexically-export branch from 0736d65 to d04885a Compare July 4, 2022 11:34
@leonerd leonerd changed the title Add builtin::lexically_export Add builtin::export_lexical Jul 4, 2022
@leonerd leonerd force-pushed the lexically-export branch from d04885a to cad26c7 Compare July 8, 2022 21:26
@leonerd leonerd changed the title Add builtin::export_lexical Add builtin::export_lexically Jul 8, 2022
@leonerd leonerd force-pushed the lexically-export branch 2 times, most recently from 9232cab to c6703ec Compare July 13, 2022 14:34
@leonerd leonerd force-pushed the lexically-export branch from c6703ec to c357ed7 Compare July 15, 2022 09:51
@leonerd leonerd merged commit fc0fc6a into Perl:blead Jul 15, 2022
@leonerd leonerd deleted the lexically-export branch July 15, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilmari ilmari ilmari approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leonerd @tonycoz @ilmari