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

Move out ValueStore from Automata and Generator classes #102

Open
3 tasks
narekgharibyan opened this issue Sep 12, 2018 · 1 comment
Open
3 tasks

Move out ValueStore from Automata and Generator classes #102

narekgharibyan opened this issue Sep 12, 2018 · 1 comment
Assignees

Comments

@narekgharibyan
Copy link
Member

narekgharibyan commented Sep 12, 2018

Implementation will provide following benefits:

  • Generator will be simplified (and mergers as well)
  • Automata class will only do FSA part.
  • Merger no longer need to be a friend of Automata
@hendrikmuhs
Copy link
Contributor

hendrikmuhs commented Sep 12, 2018

I had some thought on that and I would like to:

Create a class AutomataKeys which only loads the FSA part and provides all key related methods, that's quite useful, also for indexer. Automata will then be a subclass adding the value related methods.

Create a class DictionaryGenerator which will provide the old generator functionality, meaning creating a keyvifile for pre-sorted input. The AddValue method on the old generator will be moved to this class. This will allow us to do the final cleanup of the different value store writers. LBNL lets turn the value store into a shared pointer, right now ownership is on the generator but it should really be shared ownership between the compiler and the generator.

I am not sure about the friend relationship. We need to get the raw pointer into the value store as a performance trick to speed up merging. This is very special and I consider it unsafe for normal usage, therefore I still think this friend relationship makes sense. Fortunately #93 removed most of the other friend relationships after the separation of mergers.

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

No branches or pull requests

2 participants