Closed navision closed 6 years ago
Thanks so much for this! It looks carefully done and matches existing code styles. Wow - with unit tests too - awesome! As to the substance, a quick view shows me that you're wrapping the guts of the iteration in a function that will return key, value, or both (or possibly something else). You are obviously an experienced coder and this looks like a good idea. So that's the good news...
I need to give this a couple hours of my time, maybe a half- or a whole-day if I do comparative performance testing. This might be my most stressful 2 months in the past decade. It may take me a few weeks to adequately review this patch. I don't like waiting for my own open source patches to be accepted, but this is the best I can do right now. I'll try to merge it this weekend if I can. Your patience is appreciated.
Fixed in 3.1.0 - thank you!
I spent several hours on this today. I think it's brilliant. I reviewed it enough to find and fix my own bug in UnmodSortedMap.values() where I had declared the parent
as an UnmodMap instead of an UnmodSortedMap. This let me make the iterator
call parent.valIterator()
.
Really nice work. You clearly spent significant time understanding the code before making this fix. Passing the function saves a lot of duplicated code! I think that even Rich Hickey made special-purpose iterators for keys and values.
What motivated you to work on this, if you don't mind me asking? I don't see anything about you in a quick search. If you ever want to connect, feel free to contact me. I'd like to hear more about your programming interests.
Once any of the map types are created, the only way to inspect the keys or values ends up going through the iterator. This causes Tuple2 creation for each map entry only to possibly extract the key or value from that tuple immediately. Ideally, once a map is created, there would be a way to loop through the whole thing with minimal allocation.
The Clojure collections have previously added a keyIterator and valIterator to deal with this common occurrence. I've attached a patch with a similar implementation. key_val_iterators.patch.txt