Re: [Guile-commits] GNU Guile branch, goops-cleanup, created. release_1-9-4-72-gb1955b1

[Andy Wingo <wingo@pobox.com>]

Hi,

On Thu 05 Nov 2009 19:13, ludo@gnu.org (Ludovic Courtès) writes:

> Here’s a quick review of ‘goops-cleanup’.

Thanks very much :-))

> "Andy Wingo" <wingo@pobox.com> writes:
>
>>     * libguile/deprecated.h (scm_vtable_index_vtable): Define as a synonym
>>       for scm_vtable_index_self.
>>       (scm_vtable_index_printer): Alias scm_vtable_index_instance_printer.
>>       (scm_struct_i_free): Alias scm_vtable_index_instance_finalize.
>>       (scm_struct_i_flags): Alias scm_vtable_index_flags.
>
> IIUC these are no longer negative indices, but why deprecate them?

I think they are bad names. scm_vtable_index_vtable sounds nonsensical. 
scm_vtable_index_printer prints instances, not the vtable itself.
scm_struct_i_free is only valid on vtables, and is just a function that
runs at finalization time, and doesn't actually free anything.
scm_struct_i_flags is only valid on vtables.

>>       (SCM_STRUCTF_FLAGS): Be a -1 mask, we have a whole word now.
>>       (SCM_SET_VTABLE_DESTRUCTOR): Implement by hand.
>
> Likewise.

It is now deprecated to access flags through a mask, because the mask is
unnecessary. "Destructor" isn't mentioned anywhere else in Guile.

>>     Hidden slots.
>>     
>>     * libguile/struct.c (scm_make_struct_layout): Add support for "hidden"
>>       fields, writable fields that are not visible to make-struct. This
>>       allows us to add fields to vtables and not break existing make-struct
>>       invocations.
>
> My first reaction was that it may make the struct layout code yet
> hairier.  Would opaque fields be usable for that purpose?  In what sense
> does it attempt to “not break existing make-struct invocations”?

Imagine you have a vtable vtable with an extra field. The make-struct
invocation to make a vtable of that vtable-vtable is (make-struct
vtable-vtable layout printer extra-field). Hidden fields allow us to add
more fields to e.g. all vtables -- like a name -- without having
"extra-field" being interpreted as that extra field.

Opaque fields work but they're not readable or writable, which you want
a name to be.

It's not that bad actually.

>>     * libguile/struct.h: Lay things out cleaner. There are no more hidden
>>       (negative) words. Names are nicer. The exposition is nicer. But the
>>       basics are the same. The incompatibilities are that <vtable> has more
>>       slots now, and that scm_alloc_struct's signature has changed. The
>>       former is ameliorated by the "hidden" slots mentioned before, and the
>>       latter, well, it was always a very internal thing...
>
> Could you eventually make the log slightly more formal, describing the
> changes more than the feelings?  :-)

I don't know, that was such a big commit that it's hard to separate
things... I'll check to see if there's something more useful I can say.

> +  for (i = 0; i < n; i++)
> +    { scm_t_wchar c = scm_i_symbol_ref (layout, i*2);
>
> Could you make a pass of GNU Indent or similar,

Ah sorry, it's work's coding style infecting me... 

> and ‘c-backslash-region’ for macros like ‘SCM_CLASS_CLASS_LAYOUT’?

Sure.

> +  /* Class objects */
> +  /* if ((SCM_CLASS_FLAGS (class) & SCM_CLASSF_METACLASS)
> +      && (SCM_SUBCLASSP (class, scm_class_entity_class)))
> +      SCM_SET_CLASS_FLAGS (obj, SCM_VTABLE_FLAG_APPLICABLE); */
>
> Maybe this comment can be removed?

I'm coming back to it :)

> +  ret = (scm_t_bits)scm_gc_malloc (sizeof (scm_t_bits) * (n_words + 2), "struct");
> +  /* Now that all platforms support scm_t_uint64, I would think that malloc on
> +     all platforms is required to return 8-byte-aligned blocks. This test will
> +     let us find out quickly though ;-) */
> +  if (ret & 7)
> +    abort ();
>
> Rest assured: libgc returns 8-byte aligned data

Great! Will remove.

> -typedef void (*scm_t_struct_free) (scm_t_bits * vtable, scm_t_bits * data);
> +typedef void (*scm_t_struct_finalize) (SCM obj);
>
> I’m slightly concerned about the incompatibility.  What’s the rationale?
> (I reckon that passing ‘scm_t_bits’ pointers to user code is not very
> elegant.)

It was never documented, and only used by guile-gnome afaik. Better to
change it to do the right thing, then document it :-) I think it should
be possible to resuscitate objects too... But that's another topic.

> -     (let* ((vtable (make-vtable-vtable "pr" 0))
> +     (let* ((vtable (make-vtable "pr"))
>
> Does that mean that "hello" as a layout specifier was not detected as
> erroneous?

Yes. Later commits cause this to raise an error.

> (I’ve always thought that ‘make-vtable-vtable’ has no good raison
> d’être.  The GOOPS/CLOS model has only ‘make’, and it makes perfect
> sense to have a single procedure to “make things out of meta-things”.)

A struct is an object. A vtable is a class. A vtable-vtable is a
metaclass. Metaclasses are themselves classes; and classes are
themselves objects. You need make-vtable-vtable to make a new strange
loop at the top, like <class> being an instance of itself.

It's confusing a bit, and delightful :) See
http://wingolog.org/pub/goops-inline-slots.png.

>>     remove support for "entities" -- a form of applicable struct
>>     
>>     Entities were meant to be a form of applicable struct. Unfortunately,
>>     the implementation is intertwingled with generics. Removing them, for
>>     now, will make it possible to cleanly re-add applicable struct support.
>
> Sounds good to me.  It seems unlikely that these were used outside of
> Guile.  What do you think?

I think that's about right. But they correspond to a useful thing --
applicable structs that are not generics. They'll come back, but with a
less confusing name. (I hate that name, "entity".)

Thanks very much for the review, I'll go through this mail and poke the
branch before merging.

Andy
-- 
http://wingolog.org/