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

Add extend to imports #283

Closed
wants to merge 1 commit into from
Closed

Conversation

MarkMcCaskey
Copy link
Contributor

resolves #258 (early draft)

@MarkMcCaskey MarkMcCaskey added 🎉 enhancement New feature! 📦 lib-deprecated About the deprecated crates 📦 lib-runtime labels Mar 19, 2019
@@ -41,7 +41,13 @@ impl IsExport for Export {
/// }
/// ```
pub struct ImportObject {
map: Rc<RefCell<HashMap<String, Box<dyn LikeNamespace>>>>,
map: Rc<RefCell<HashMap<(String, String), Box<dyn IsExport>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Main issue with this is that we need to keep imported instances alive. I'm not sure the best way to do this, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not familiar enough with the code base yet to understand how this change affects the lifetimes of the Exports. It seems like it should be the same, unless you mean that we were dropping entries by namespace before 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the Box<dyn LikeNamespace> could contain an Instance. When getting a function pointer from an ImportObject, if the Instance isn't maintained in the ImportObject, the function pointer will dangle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it's possible to use two maps and check both to emulate a global name lookup.

Though, I'm still a bit unclear on the ownership... ImportObjects own Instances and Instances own ImportObjects?

@syrusakbary
Copy link
Member

Closing this in favor of #286

bors bot added a commit that referenced this pull request Mar 25, 2019
286: Add extend to imports  r=MarkMcCaskey a=MarkMcCaskey

a rewrite of #283 with a focus on simplicity

resolves #258

Co-authored-by: Mark McCaskey <[email protected]>
@MarkMcCaskey MarkMcCaskey deleted the feature/add-extend-to-imports branch April 25, 2019 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-deprecated About the deprecated crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to combine two import objects
3 participants