Skip to content

Reconcile table ownership semantics #439

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
molpopgen opened this issue Jan 8, 2023 · 3 comments · Fixed by #448
Closed

Reconcile table ownership semantics #439

molpopgen opened this issue Jan 8, 2023 · 3 comments · Fixed by #448
Milestone

Comments

@molpopgen
Copy link
Member

We currently have two representations of each low-level table -- one that owns its data and one that does not.
This design is arguably confusing and arose primarily over concerns about memory safety.
However, it should be fixed to give a clearer API.

We should be able to write something like:

pub struct EdgeTable(TableManager<tsk_edge_table_t>)

where the manager type is responsible for ownership.
Then, EdgeTable is just a smart pointer and can Deref/DerefMut safely to the manager's public API.

@molpopgen molpopgen added the API breaking Issues/PRs that may break API. label Jan 8, 2023
@molpopgen molpopgen added this to the 0.13.0 milestone Jan 8, 2023
@molpopgen
Copy link
Member Author

To prevent any immediate API breakage, we can supply type aliases for the owning table types.
Marking the aliases as deprecated will cause client code lints to pick up on the change.

@molpopgen molpopgen removed the API breaking Issues/PRs that may break API. label Jan 26, 2023
@molpopgen
Copy link
Member Author

This is not API breaking after all! The OwningXTable can be a type alias for XTable. This alias will be deprecated in 0.13.0 and removed "eventually".

@molpopgen
Copy link
Member Author

#448 managed to do this w/o appealing to any Deref/DerefMut!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant