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

Use methods rather than functions #13

Closed
tmpfs opened this issue Aug 12, 2022 · 1 comment · Fixed by #15
Closed

Use methods rather than functions #13

tmpfs opened this issue Aug 12, 2022 · 1 comment · Fixed by #15

Comments

@tmpfs
Copy link
Collaborator

tmpfs commented Aug 12, 2022

The API feels a bit strange calling add_document_to_index(), remove_document_from_index() and vacuum_index() as they all accept &mut index as the first parameter.

Whilst we are making breaking changes I suggest using methods on Index:

impl Index {
  fn add_document(&mut self, ..) {}
  fn remove_document(&mut self, ..) {}
  fn vacuum(&mut self, ..) {}
}

I think this is more idiomatic. What do you think?

@marcus-pousette
Copy link
Collaborator

marcus-pousette commented Aug 12, 2022

Yes you are right! At line 34 there is also already an impl for Index, so that makes a lot of sense! I think the reason for this, is that I was playing around with different memory layouts for the Index when building this library. There is a ghost_cell implementation that is about 30% more faster (if I remember correctly) in the ghost branch of this repo, where I think this, for some reason was justified. But I never succeeded to make the ghost_cell implementation compatible with WASM that requires you to be able to wrap the index with Mutex<...>.

In all, the change you suggest is preferred and is more idiomatic

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

Successfully merging a pull request may close this issue.

2 participants