geda-developers team mailing list archive

[Peter TB Brett <peter@xxxxxxxxxxxxx>]

On Wednesday 19 December 2012 00:00:27 Ivan Stankovic wrote:
> On Mon, Dec 17, 2012 at 04:31:43PM +0000, Peter TB Brett wrote:
> > #define EDA_TYPE_OBJECTSET (eda_object_set_get_type ())
> 
>                                         ^
> You probably want to remove that underscore.
> 
> Also, EdaObjectset looks a bit weird, compared to naming conventions
> in other libraries, but I don't have a better name.

It does. :-( If we were using a "proper" OO system, I don't think that 
we would be worrying about this (look at the Java Collections API "Set" 
interface for example).

> > struct _EdaObjectsetClass
> > {
> > 
> >   GObjectClass parent_class;
> >   
> >   /* virtual public methods */
> >   gboolean (*get_iter)(EdaObjectset *set, EdaObjectIter *iter, gint
> >   index);
> I assume the index here is the initial iterator position.

Yes.

> >   gboolean (*find)(EdaObjectset *set, EdaObjectIter *iter);
> 
> Missing OBJECT * here.

Good point.

> >   OBJECT *(*get)(EdaObjectset *set, EdaObjectIter *iter);
> >   void (*append)(EdaObjectset *set, OBJECT *object);
> >   void (*insert)(EdaObjectset *set, EdaObjectIter *iter, OBJECT
> >   *object); void (*remove)(EdaObjectset *set, EdaObjectIter *iter,
> >   OBJECT *object);
> Looking good (but I wonder about subclasses and how they might
> implement these).

I introduced "append" as distinct from "insert" because it's impossible 
to obtain an iterator pointing beyond the end of the set.

> > /* Convenience functions */
> > EdaObjectset *eda_objectset_new (void) G_GNUC_WARN_UNUSED_RESULT;
> > void eda_objectset_foreach (EdaObjectset *set, GCallback func,
> > gpointer user_data);
> > 
> > /* Getting iterators */
> > gboolean eda_objectset_get_iter (EdaObjectset *set, EdaObjectIter
> > *iter, gint index); gboolean eda_objectset_iter_next (EdaObjectset
> > *set, EdaObjectIter *iter); gboolean eda_objectset_find
> > (EdaObjectset *set, EdaObjectIter *iter, OBJECT *object);
> > 
> > /* Obtaining values */
> > OBJECT *eda_object_set_get (EdaObjectset *set, EdaObjectIter *iter);
> 
>                     ^
> Extra undescore.
> 
> > /* Modifying the set */
> > void eda_objectset_append (EdaObjectset *set, OBJECT *object);
> > void eda_objectset_insert (EdaObjectset *set, EdaObjectIter *iter,
> > OBJECT *object); void eda_objectset_remove (EdaObjectset *set,
> > EdaObjectIter *iter);
> Looking good, but I expect the eda_objectset_remove method will be
> cumbersome to use.  You would first need to obtain the iterator that
> points to the object and then call eda_objectset_remove. For example,
> eda_objectset_append does not require an iterator.

Okay, so as mentioned above, "append" probably should require an 
iterator -- it'll append an object to the set and then set the iterator 
to point to the new location in the set.

> APIs are best judged by examples so let's say that I want to remove
> all objects from a page that satisfy some criteria:
> 
>     EdaObjectIter iter;
>     EdaObjectset *set = eda_page_get_objects(page);
> 
>     if(eda_objectset_get_iter(set, &iter, 0)) {
>         while(1) {
>             EdaObject *obj = eda_objectset_get(set, &iter);
> 
>             if(should_remove(obj))
>                 eda_objectset_remove(set, &iter);
> 
>             if(!eda_objectset_iter_next(set, &iter))
>                 break;
>         }
>     }
> 
> 
> While this is not too verbose, I have a feeling we could do better.

Yes, 

> More troublesome is that it's not obvious how eda_objectset_remove
> and eda_objectset_iter_next interact. Does eda_objectset_remove
> automatically advances the iterator? Does the iterator get
> invalidated?

Yes, "remove" will make sure that the iterator is updated to point to 
the entry after the one which was removed. That means that it's actually 
*possible* to remove all objects that satisfy particular criteria from 
the set in O(N) rather than O(N^2) time without making a copy of the 
set.  For example, how would you implement the algorithm above using an 
eda_objectset_remove() that doesn't use an iterator?

This discussion all seems a bit daft... I feel like this is exactly the 
sort of thing that should be provided by a basic library.

                                 Peter

-- 
Peter Brett <peter@xxxxxxxxxxxxx>
Remote Sensing Research Group
Surrey Space Centre

Attachment: signature.asc
Description: This is a digitally signed message part.