[newchem-cpp] Introduce FrozenKeyIdxBiMap
#451
+489
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is some unforunate consequences, but if we don't do this you end up with some pretty confusing looking code... (the confusion primarily arises if new_FrozenKeyIdxBiMap or drop_FrozenKeyIdxBiMap has different behavior from other functions with similar names). I really think it would be better to make this into a simple C++ class with a constructor and destructor, but thats a topic for another time
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces a data structure called
FrozenStringIdxBiMap. This is a bidirectional map (aka a bidirectional dictionary), that can be used to map between a unique set ofnstrings (keys) and a unique set of indexes (with values of0throughn-1) and vice-versa.Note
The implementation of the data structure in this PR is overly simplistic (it has
O(N)complexity rather thanO(1)complexity). This was intentional to make the PR easier to review.PR #270 proposed a very similar data structure that has a much faster implementation. It's on my todo list to redo PR #270 and replace the implementation proposed here (I'm just going to need to make a few tweaks to creation/destruction functions so that the API is consistent with what we propose in this PR)
I intend to use this as a building block to implement other types (namely types with a map-like interface) and initialization code. There are some obvious applications related to the dynamic API. There are a couple of times where I have wanted this kind of data structure and instead I wrote crude workarounds
The underlying implementation is based on a hash table. To implement this, I ported a class (from C++ to C) that I previously implemented in Enzo-E to perform the same function (called StringIndRdOnlyMap). In addition to porting the logic from C++ to C1, I also added logic to handle a few edge cases. For context, I originally wrote the Enzo-E class so that to make use of more specialized logic, which should make this faster that a solution that makes use of more generic types like
std::mapfrom the C++ standard library.Footnotes
With the benefit of hindsight, the effort to convert from C++ to C took way more time than I expected and it probably wasn’t worth it. In fact, it may make more sense ↩