You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucy.apache.org by Timothy Wilkens <ti...@gmail.com> on 2014/09/01 01:27:41 UTC
Re: [lucy-dev] Review of HashIterator pull request
On Sun, Aug 31, 2014 at 1:43 PM, Nick Wellnhofer <we...@aevum.de>
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
Re: [lucy-dev] Review of HashIterator pull request
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Wed, Sep 3, 2014 at 10:19 AM, Timothy Wilkens <ti...@gmail.com> wrote:
> I have re-submitted my pull request with the following changes:
The revised pull request looks good to me and I plan to merge it to
master in a day or so.
Marvin Humphrey
Re: [lucy-dev] Review of HashIterator pull request
Posted by Timothy Wilkens <ti...@gmail.com>.
Hello all,
I have re-submitted my pull request with the following changes:
1. Get_Key and Get_Value now throw errors when they are called before
iteration has started or after iteration has ended.
2. Get_Key checks for the Tombstone and throws an error since this only
occurs if the hash has been modified during iteration.
3. Get_Value has been updated to be nullable.
Notably, I have left the HashIterator code inside separate source files,
rather than moving it into Hash. I felt that without a strong consensus it
made more sense to leave as is and update later if that's the pattern we
decide on.
Tim
On Mon, Sep 1, 2014 at 10:43 AM, Nick Wellnhofer <we...@aevum.de>
wrote:
> On Sep 1, 2014, at 19:27 , Marvin Humphrey <ma...@rectangular.com> wrote:
> > There's also knowing what header file corresponds to a given public
> class,
> > since Clownfish does not at present impose any restriction on where
> classes
> > may be defined.
>
> I don’t want to lose the ability to share a .c file for multiple classes.
> We could simply have multiple .cfh files and the implementation in a single
> .c file. But this might be confusing.
>
> > A related bug is that we do not enforce that Hash keys must be immutable.
> >
> > Hash *hash = Hash_new(0);
> > ByteBuf *foo = BB_new_bytes("foo", 4)
> > Hash_Store(hash, (Obj*)foo, NULL);
> > VArray *keys = Hash_Keys(hash);
> > ByteBuf *key = (ByteBuf*)VA_Fetch(keys, 0);
> > BB_Set_Size(key, 2); // Uh oh.
> >
> > There's a straightforward solution, though. 99% of the time, Hash uses
> only
> > String keys. String is immutable. We should consider requiring Hash
> keys to
> > be Strings. There are a lot of advantages, and few downsides.
>
> Or we clone the returned keys like we clone them in Hash_Make_Key.
>
> Nick
>
>
Re: [lucy-dev] Review of HashIterator pull request
Posted by Nick Wellnhofer <we...@aevum.de>.
On Sep 1, 2014, at 19:27 , Marvin Humphrey <ma...@rectangular.com> wrote:
> There's also knowing what header file corresponds to a given public class,
> since Clownfish does not at present impose any restriction on where classes
> may be defined.
I don’t want to lose the ability to share a .c file for multiple classes. We could simply have multiple .cfh files and the implementation in a single .c file. But this might be confusing.
> A related bug is that we do not enforce that Hash keys must be immutable.
>
> Hash *hash = Hash_new(0);
> ByteBuf *foo = BB_new_bytes("foo", 4)
> Hash_Store(hash, (Obj*)foo, NULL);
> VArray *keys = Hash_Keys(hash);
> ByteBuf *key = (ByteBuf*)VA_Fetch(keys, 0);
> BB_Set_Size(key, 2); // Uh oh.
>
> There's a straightforward solution, though. 99% of the time, Hash uses only
> String keys. String is immutable. We should consider requiring Hash keys to
> be Strings. There are a lot of advantages, and few downsides.
Or we clone the returned keys like we clone them in Hash_Make_Key.
Nick
Re: [lucy-dev] Review of HashIterator pull request
Posted by Marvin Humphrey <ma...@rectangular.com>.
On Mon, Sep 1, 2014 at 2:43 AM, Nick Wellnhofer <we...@aevum.de> wrote:
> The main argument not to implement multiple classes in the same file is to
> have a better separation of concerns.
There's also knowing what header file corresponds to a given public class,
since Clownfish does not at present impose any restriction on where classes
may be defined.
But imports are a larger question that we need to sort out; I'll take that up
in another thread later.
> I don't want to change the way the end of iteration is detected. Next should
> still return false after the iterator is finished. I only propose that once
> Next has returned false, calls to Get_Key and Get_Value shouldn't be
> allowed. This wouldn't affect the standard idiom:
>
> // Works like before.
> HashIterator *iter = HashIter_new(hash);
> while (HashIter_Next(iter)) {
> Obj *key = HashIter_Get_Key(iter);
> Obj *value = HashIter_Get_Value(iter);
> }
> // Calling Get_Key or Get_Value now would throw an exception.
>
> If someone calls Get_Key or Get_Value after the end of iteration, it's most
> likely a programming error, and throwing an exception helps to detect this.
+1
Hash does not allow NULL keys. It is useful to propagate that invariant to
HashIter_Get_Key(), because it eliminates a need for NULL checking in client
code.
A related bug is that we do not enforce that Hash keys must be immutable.
Hash *hash = Hash_new(0);
ByteBuf *foo = BB_new_bytes("foo", 4)
Hash_Store(hash, (Obj*)foo, NULL);
VArray *keys = Hash_Keys(hash);
ByteBuf *key = (ByteBuf*)VA_Fetch(keys, 0);
BB_Set_Size(key, 2); // Uh oh.
There's a straightforward solution, though. 99% of the time, Hash uses only
String keys. String is immutable. We should consider requiring Hash keys to
be Strings. There are a lot of advantages, and few downsides.
Marvin Humphrey
Re: [lucy-dev] Review of HashIterator pull request
Posted by Nick Wellnhofer <we...@aevum.de>.
On 01/09/2014 01:27, Timothy Wilkens wrote:
> 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.
Well, right now, users have to look into the .cfh files for documentation. So
the decision whether classes are implemented in a single file or not might
have a small impact on how the role of a class is perceived. But once we start
generating docs for the C API, it won't make a difference.
The main argument not to implement multiple classes in the same file is to
have a better separation of concerns. But Hash and HashIterator are so tightly
coupled that I don't see any benefits from splitting up the implementation.
> 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.
I don't want to change the way the end of iteration is detected. Next should
still return false after the iterator is finished. I only propose that once
Next has returned false, calls to Get_Key and Get_Value shouldn't be allowed.
This wouldn't affect the standard idiom:
// Works like before.
HashIterator *iter = HashIter_new(hash);
while (HashIter_Next(iter)) {
Obj *key = HashIter_Get_Key(iter);
Obj *value = HashIter_Get_Value(iter);
}
// Calling Get_Key or Get_Value now would throw an exception.
If someone calls Get_Key or Get_Value after the end of iteration, it's most
likely a programming error, and throwing an exception helps to detect this.
Nick