OakHashMap Api #161
OakHashMap Api #161
Conversation
|
|
||
|
|
||
|
|
||
| private final K fromKey; |
There was a problem hiding this comment.
If you bother those those "to be removed" fields you can add a //TODO: comment saying it will be removed as soon as internalOakHashMap introduced to this code
There was a problem hiding this comment.
Also it can be initialized here instead of constructor
| this.valuesMemoryManager, kMM, chunkMaxItems, new ValueUtils()); | ||
|
|
||
|
|
||
| this.fromKey = null; |
There was a problem hiding this comment.
Thinking deeper about it, assigning all those parameters to undefined value, making it impossible to do parametrized testing for iterators... @liran-funaro what do you think to rewrite iterator's invocation code for OakHash inside ConcurrentZCMap interface?
|
@OrHayat , excellent! Is it the end of correction of the previous code review comments? |
|
yes but it still dont pass the coverage tests probably because some tests used submap so couldnt be parametrized.
|
|
Thank you Or! We will review the code regardless to the check failure. |
| return valuesMemoryManager; | ||
| } | ||
|
|
||
| public static class OakZeroCopyMap<K, V> implements ZeroCopyMap<K, V> { |
There was a problem hiding this comment.
Why the same name as in OakMap class? Is it legitimate at all?
There was a problem hiding this comment.
@liran-funaro can a template be made for OakMap vs OakHashMap?
There was a problem hiding this comment.
I don't see why not. I think we should try to do it.
There was a problem hiding this comment.
Looks good to me.
Note that there are some places we can have more code-reuse than we do (e.g., the KeySet/EntrySet classes).
Kudos for the effort in parametrizing the tests. It is very important for the development effort.
I think parametrizing the API test should be sufficient to cover the rest of the uncovered lines.
Please let us know if you have the capacity for these additional changes.
Otherwise, we'll need to start working on it ourselves as the time is pressing on this.
| return valuesMemoryManager; | ||
| } | ||
|
|
||
| public static class OakZeroCopyMap<K, V> implements ZeroCopyMap<K, V> { |
There was a problem hiding this comment.
I don't see why not. I think we should try to do it.
|
|
||
| /* ---------------- View Classes -------------- */ | ||
|
|
||
| static class KeySet<K> extends AbstractSet<K> { |
There was a problem hiding this comment.
Can we reuse the code of KeySet, EntrySet, and the others from OakMap?
There was a problem hiding this comment.
not easily because they are inner classes of the respective classes because you wanted the iterators of the OakHashMap and OakMap functions to be private and be avilable only from the code of KeySet and such.
|
Dear Or (@OrHayat ) thank you very much for your significant contribution! It was a pleasure to work with you! Hope to continue collaboration! |
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.
#141