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

[Ludovic Courtès]

Hello Guilers!

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

"Andy Wingo" <address@hidden> writes:

> - Log -----------------------------------------------------------------
> commit b1955b1187eccb9383154942f0353d5cdcfeed99
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 23:59:51 2009 +0100
>

[...]

>     Applicable struct runtime support.

Sounds nice.

[...]

>     * 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?

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

Likewise.

>     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”?

[...]

>     * 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?  :-)


[...]

>     Remove foreign object implementation.

Good.


[...]

>     Clean up goops.h, a little.

OK.

Besides:

+  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, and ‘c-backslash-region’
for macros like ‘SCM_CLASS_CLASS_LAYOUT’?

+  /* 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?

+  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 (otherwise pairs
wouldn’t work).  See
http://thread.gmane.org/gmane.comp.programming.garbage-collection.boehmgc/1272 .

-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.)

[...]

> commit ba33a21a28ceb5cf5a30ca34d4f833b8e3292187
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:43:29 2009 +0100
>
>     actually set all 8 hashsets in classes
>     
>     * libguile/goops.c (prep_hashsets): Actually set all 8 hashsets. Doh...
>
> commit 0fb03157867ae1d12257928f9253c19b267e8ede
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:41:59 2009 +0100
>
>     remove goops-local %tag-body
>     
>     * libguile/goops.c (scm_sys_tag_body): Remove goops-local hack that is
>       no longer necessary.
>
> commit 84dab750a7db5e451b4bc6e49652161d298621a9
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:33:22 2009 +0100
>
>     SCM_VALIDATE_VTABLE tweak
>     
>     * libguile/validate.h (SCM_VALIDATE_VTABLE): Simply call
>       scm_struct_vtable_p.
>
> commit d427d8386ad5d23332280d60eaef7226620513bf
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:32:39 2009 +0100
>
>     fix printing-programs bug
>     
>     * module/system/vm/program.scm (write-program): Fix a bug if we couldn't
>       get a procedure's arity.
>
> commit 48a69b27398d88dd54965b7584191acf2a974fde
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:28:26 2009 +0100
>
>     values.c defines a vtable, not a vtable-vtable
>     
>     * libguile/values.c (scm_init_values): Define a vtable, not a
>       "metaclass".
>
> commit d8ebd1d41d3cfe0aab118c214dc7db4b0ac3c433
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:27:34 2009 +0100
>
>     srfi-35 properly uses vtable-offset-user
>     
>     * module/srfi/srfi-35.scm (condition-type-id):
>       (condition-type-parent, condition-type-all-fields): Don't ref fixed
>       struct indices, use vtable-offset-user instead.

OK up to here.

> commit 635bc2288076acfa5fcd305704d2a95b882fa090
> Author: Andy Wingo <address@hidden>
> Date:   Tue Nov 3 22:26:46 2009 +0100
>
>     fix a structs.test test
>     
>     * test-suite/tests/structs.test ("equal?"): Instantiate vtables, not
>       vtable-vtables. "hello" is not a valid vtable layout.

-     (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?  Or is there something more subtle going on here?

(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”.)

> commit 67d29780cd0288bb8b6825ac9bfe207a3b5192b8
> Author: Andy Wingo <address@hidden>
> Date:   Sun Nov 1 21:09:42 2009 +0100
>
>     remove redundant, unnecessary instance size from struct vtables
>     
>     * libguile/struct.h (scm_struct_i_size): Remove this shared field -- I
>       mean, the slot is still there, but it's only used for flags.
>     
>     * libguile/goops.h (SCM_SET_CLASS_INSTANCE_SIZE):
>     * libguile/goops.c (scm_sys_inherit_magic_x, scm_make_class): Remove
>       uses and definition of SCM_SET_CLASS_INSTANCE_SIZE. Light structs used
>       it, but you have that info in the layout; and foreign classes used it,
>       but that is going away soon anyway :)
>
> commit 2f145cc2974c1b528fed5e1cc7855d553f084c57
> Author: Andy Wingo <address@hidden>
> Date:   Sun Nov 1 20:45:02 2009 +0100
>
>     fold objects.[ch] into goops.[ch]
>     
>     Remove objects.h #includes as appropriate.
>
> commit 5ea6ce3cde873cb75e4eddd36781182940177d68
> Author: Andy Wingo <address@hidden>
> Date:   Sun Nov 1 19:46:27 2009 +0100
>
>     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?


[...]

> commit 0dccce8b8ab89c56b0816801b7700d1bed80ee8c
> Author: Andy Wingo <address@hidden>
> Date:   Sat Oct 31 12:04:53 2009 +0100
>
>     SCM_GENERIC_METHOD_CACHE macro splits from SCM_ENTITY_PROCEDURE
>     
>     * libguile/goops.h (SCM_GENERIC_METHOD_CACHE)
>       (SCM_SET_GENERIC_METHOD_CACHE): Two new macros; the same as
>       SCM_[SET_]ENTITY_PROCEDURE, but more reflecting the reality of the
>       generic hack.
>     
>     * libguile/eval.i.c:
>     * libguile/goops.c:
>     * libguile/objects.c:
>     * libguile/vm-i-system.c: Use the new macros when it is appropriate to
>       do so.
>
> commit 87af2dfc34cb5228de3574057a277df930581547
> Author: Andy Wingo <address@hidden>
> Date:   Sat Oct 31 11:17:12 2009 +0100
>
>     remove unused things from object.[ch]
>     
>     * libguile/objects.h:
>     * libguile/objects.c (scm_object_procedure): Remove, it was only
>       compiled with SCM_DEBUG.
>     
>     * libguile/objects.h:
>     * libguile/objects.c (scm_make_class_object, scm_make_subclass_object,
>       (scm_i_make_class_object, scm_metaclass_standard): Remove also. These
>       implemented an undocumented object system, and are totally replaced by
>       GOOPS.
>
> commit 90dcbb12be64260cb1329c456a4f02900639347d
> Author: Andy Wingo <address@hidden>
> Date:   Sat Oct 31 00:28:43 2009 +0100
>
>     remove operators
>
> commit a1724f53c0e51d5577d8e50d25867abc010991d0
> Author: Andy Wingo <address@hidden>
> Date:   Sat Oct 31 00:08:42 2009 +0100
>
>     generic dispatch in the vm (sorta)
>     
>     * libguile/vm-i-system.c (call, goto/args, mv-call): Add a case for
>       generics, so we can avoid the evaluator in that case. Still have to
>       cons up a list -- the real solution comes later.
>
> commit 0108ba52a1da29e6b0f84c6380a5d4b36311a271
> Author: Andy Wingo <address@hidden>
> Date:   Thu Oct 29 09:47:00 2009 +0100
>
>     remove unused struct gc chain macros
>     
>     * libguile/struct.h (SCM_STRUCT_GC_CHAIN, SCM_SET_STRUCT_GC_CHAIN):
>       Remove, no longer necessary given topological finalization, provided
>       by libGC.
>
> commit 8fa54cfd403e82ae4818c817036106e9f13e90e3
> Author: Andy Wingo <address@hidden>
> Date:   Fri Oct 30 22:21:29 2009 +0100
>
>     de-inline goops dispatch from the evaluator

Perfect!

Keep up the good work!

Ludo’.