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

Allow modules to optionally import nothing at all - final bits #42154

Merged
merged 2 commits into from
Sep 17, 2021

Conversation

cmcaine
Copy link
Contributor

@cmcaine cmcaine commented Sep 8, 2021

I never got around to addressing Jeff's suggestion that empty modules should not contain a reference to themselves.

This PR (which I wrote in March but didn't push for some reason D:) addresses that and also tweaks the interface and the docs slightly.

NEWS.md Outdated
Comment on lines 8 to 9
* `(; a, b) = x` can now be used to destructure properties `a` and `b` of `x`. This syntax is equivalent to `a = getproperty(x, :a)`
and similarly for `b`. ([#39285])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was in my old branch, but not in master, but I think it should still be in the master branch, perhaps it was removed accidentally?

Copy link
Member

Choose a reason for hiding this comment

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

The feature is added for v1.7, and this is a release note for v1.8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so it is. Well, perhaps I should merge this with a different branch or against a different tag or something?

Maybe 7ca1a90 ? (one commit before Kristoffer Carlsson updated VERSION)?

Copy link
Member

Choose a reason for hiding this comment

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

Please rebase on master. We first merge things against master and later one backport if needed.

@cmcaine
Copy link
Contributor Author

cmcaine commented Sep 8, 2021

This should be in 1.7 but master has moved on. Very sorry for the inconvenience.

Let me know if you'd like me to rebase these changes onto e.g. 7ca1a90 (one commit before Kristoffer Carlsson updated VERSION) or if you'd like me to present a smaller changeset (e.g. only fixing src/module.c so that empty modules don't contain a reference to themselves).

@@ -428,7 +428,8 @@ eval(Core, :(InterConditional(slot::Int, @nospecialize(vtype), @nospecialize(els
eval(Core, :(MethodMatch(@nospecialize(spec_types), sparams::SimpleVector, method::Method, fully_covers::Bool) =
$(Expr(:new, :MethodMatch, :spec_types, :sparams, :method, :fully_covers))))

Module(name::Symbol=:anonymous, std_imports::Bool=true, using_core::Bool=true) = ccall(:jl_f_new_module, Ref{Module}, (Any, Bool, Bool), name, std_imports, using_core)
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 breaking, since it removes the 3-argument version. We should still allow it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Would you prefer that I leave the Julia interface unchanged and just fix src/module.c so that empty modules don't include their own name?

Is it okay if I rename the argument using_core to default_names?

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I didn't realize this hadn't been released yet, so it's not actually breaking. The current version of the PR is fine with me though.

@cmcaine
Copy link
Contributor Author

cmcaine commented Sep 10, 2021

Rebased into two commits on master. I've left the Julia interface unchanged from #40110 because it doesn't seem worth it to change it.

@JeffBezanson JeffBezanson merged commit 8183e6a into JuliaLang:master Sep 17, 2021
KristofferC pushed a commit that referenced this pull request Sep 28, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants