Hi, On Thu 05 Nov 2009 19:13, [email protected] (Ludovic Courtès) writes:
> Here’s a quick review of ‘goops-cleanup’. Thanks very much :-)) > "Andy Wingo" <[email protected]> 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/
