On Tue, 2008-08-12 at 14:15 +0200, Bernd Jendrissek wrote:
> On Mon, Aug 11, 2008 at 10:08 PM, Peter Clifton <[EMAIL PROTECTED]> wrote:
> > I don't really object, but forcing clients to know the size of objects
> > to allocate them isn't making things opaque either, which was the
> > "title" of the patch series.
>
> Where do you get the idea that I'm forcing clients to know the size of
> objects???
>From your comments, I guess it must have come across that I was against
this patch. I'm not, really! WRT. size of objects:
In that specific inheritance example, libgeda would have to divulge how
big its own OBJECT structure is, which is somewhat contrary to the type
being opaque. If the OBJECT type were to be made opaque outside of
libgeda, gschem couldn't subclass it in that way, it would have to:
typedef struct {
OBJECT *owned_opaque_object; // (NB: OBJECT *, not OBJECT).
int extra_stuff;
} sortof_subclassed_object;
If it was OBJECT, not OBJECT *, then OBJECT can't really be opaque. At
the very least, it would have to be exported as a dummy structure with
the same size as the libgeda internal OBJECT representation.
> > My preference for object API (s_basic_init_object aside) is along these
> > lines:
> >
> > OBJECT *o_box_new (...);
> > OBJECT *o_line_new (...);
>
> Well my more elaborate factory pattern does almost exactly this. Have
> you seen it?
I haven't read the code in detail recently, but I did read it at the
time of original posting.
> The more elaborat s_toplevel_new_object() has a switch
> on object type, which then calls to sub-methods for each specific
> State object. ("State" is the pattern libgeda uses to distinguish
> object types; things like LINE and PICTURE are the states. See
> http://c2.com/cgi/wiki?StatePattern for more info.)
I'm not familiar with all these (what I would term "compsci") OO pattern
names, so thanks for providing the link. I can grok "MVC", "factory" (I
think), I think I inferred what "delegation" was, etc..
> > I'd not object to something like you proposed, a bit more GObject
> > subclassing like, e.g.:
> >
> > typedef struct {
> > OBJECT object;
> > int x[2], y[2];
> > } LINE;
>
> I prefer this approach to the State pattern we have now. We don't
> need dynamic classification, hence we don't need the State pattern. I
> think the original reason for doing it the way we do is that Ales
> didn't like the idea of shoehorning different-sized data into one
> struct definition. It seems ugly to me too.
You mean using union types, or just providing every field for every type
of object within OBJECT?
> > I didn't say I was against the patch, only that I didn't like the idea
> > of gschem sub-classing libgeda types due to concerns over object
> > ownership.
>
> Why "couldn't [you] apply this patch in good conscience" then?
Wasn't me that said that, I don't mind the patch which splits
allocation / initialisation.
A few comments though:
s_toplevel_new_object () probably isn't the best name. It doesn't
operate on a TOPLEVEL, so it shouldn't be s_toplevel_..., and should not
live in s_toplevel.c. Why not call it s_basic_new_object () and place in
s_basic.c, for consistency with the old code.
I see TOPLEVEL was added as a parameter to s_toplevel_new_object ()
either. Is that just for consistency with other API calls? Why not leave
it off, since it isn't used. It keeps down the delta against how
s_basic_init_object() used to be called.
It might be useful to reword the commit message as well:
>
> s_basic_init_object() now only initializes an object. It is up to the
> object factory to allocate as much memory as it knows the object (which
> could be a derived class) really needs. Add a helper function,
> s_toplevel_new_object() that wraps the calls to the factory and to
> s_basic_init_object().
>
> [For "object factory", read: s_toplevel_new_object(). A real (abstract)
> factory is too controversial to advocate for the mainstream code.]
What about:
"
Split memory allocation for OBJECTs out of s_basic_init_object(). Add a
new helper function s_basic_new_object() which allocates memory for the
OBJECT structure, then calls s_basic_init_object().
These changes make it possible to embed an OBJECT structure within
another struct, or to initialise an OBJECT in static memory.
"
Talk of object factories, and [...] comment is only serve to obfuscate
what the patch does with "compsci" terms which not everyone (myself
included) will always grok.
> The holy grail is opaque objects. The first non-trivial use of an
> abstract factory might be to sequester knowledge of GdkPixbuf into gschem
> and out of libgeda. Conceptually, gschem would override the abstract
> factory's ::new_picture() method, returning a bigger structure than just
> PICTURE:
> typedef struct st_gschem_picture GSCHEM_PICTURE;
> struct st_gschem_picture {
> PICTURE base;
> GdkPixbuf *original_picture;
> GdkPixbuf *displayed_picture;
> /* Maybe some other stuff too. */
> };
>
Moving GdkPixbuf into gschem is a laudible goal, but more related to
abstraction / delegation / clear separation of code. It doesn't rely on
anything being opaque at all. Can we drop that comment?
It would also be nice to drop comments about factories from the patched
code, e.g.:
- * The <B>OBJECT</B> structure is allocated with the #s_basic_init_object()
+ * The <B>OBJECT</B> structure is allocated with the object factory.
could / should become:
- * The <B>OBJECT</B> structure is allocated with the #s_basic_init_object()
+ * The <B>OBJECT</B> structure is allocated with #s_basic_new_object()
Actually, IMO, these (existing) doxygen comments are rather verbose. It
isn't necessary to have them describe blow by blow how it does what it
does, that whole paragraph about allocation and init could be removed.
(Something for another patch though).
> >> Heck, I wouldn't even mind if libgeda's OBJECT definition had extra
> >> void * members specifically for gattrib / gschem / etc. It's not like
> >> libgeda is the system C library with 10000 dependent packages!
> >
> > True, but it would only take one client app wanting to stash data to
> > break this. It would have to stash it's own mapping table, or take a
> > guess abuse (re-use) one of the other void * pointers.
>
> WHEN such a client app appears we can add that abstraction. I'm all
> for lazy evaluation here! Do you have a specific application in mind
> that would like to stash its own data in OBJECT? Maintaining its own
> mapping table doesn't seem to onerous a requirement to me.
You're right of course, I was probably over-designing, and I don't for a
second expect libgeda API to stay stable between versions at this time.
PS.. unless we can come up with a solution for printing, GdkPixbuf will
have to stay in libgeda, since that is where the postscript output code
lives. :(
--
Peter Clifton
Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA
Tel: +44 (0)7729 980173 - (No signal in the lab!)
_______________________________________________
geda-dev mailing list
[email protected]
http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev
