On Sun, Aug 31, 2014 at 1:43 PM, Nick Wellnhofer <[email protected]> wrote:
> 1. If HashIterator was implemented directly in Hash.c, it would have > access to the static global for the hash tombstone and the HashEntry > struct. This would simplify the code. I had initially placed HashIterator inside Hash.c but after some discussion with Marvin I decided to move it, even though it caused the implementation to be complicated. I believe that having it outside of Hash.c highlights the fact that hash iteration is done through a separate object now. 2. Get_Key and Get_Value check whether the current key is a tombstone. This > can happen if the hash was modified during iteration. I’d probably have > missed this corner case, but if this happens, I’d throw instead of > returning NULL. +1. It had not occurred to me that seeing a tombstone in Get_Key means that the hash has been modified after the call to Next. Another thing, which is more of a discussion item, is whether Get_Key and > Get_Value should throw or return NULL if Next hasn’t been called yet or the > iterator is finished. I don't feel strongly either way with respect to calling Get_Key or Get_Value before Next has been called, though I'm leaning towards agreeing with you and throwing an error. After iteration is finished I believe that returning false from Next and NULL from Get_Key and Get_Value is the desired behavior. It seems more natural to me to continue iteration until Next returns false, rather than checking the state of iteration before calling Next to avoid throwing an error. Get_Value should be made “nullable”, by the way. Agreed. Thanks for the feedback. Tim
