Elisp printer (was: bug#25295: Represent eieio objects using object-print in backtraces and edebug)

Elisp printer (was: bug#25295: Represent eieio objects using object-print in backtraces and edebug)

[Michael Heerdegen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Maybe we should just switch to an Elisp version of printing, in that
> case.

That's a very nice idea, not just for the eieio case.


> ;;; cl-print.el --- Generic printer facilies         -*- lexical-binding: t; -*-

Does "cl" stand for "command line" or for "Common Lisp"?  I find the
name hard to remember in both cases, because that's not what comes to my
mind when I think of this library (when I internet-search for
"cl-print", I find mostly shops that print business cards and such
stuff...)

Anyway, we really really need to define a printer for thunk.el thunks,
and maybe an extra printer for streams (that's why I CC Nicolas), and
probably some similar other stuff!


Michael.

Re: Elisp printer

[Stefan Monnier]

> Does "cl" stand for "command line" or for "Common Lisp"?

It's for "Common Lisp", since it uses the Common Lisp names and
calling conventions (as well as reliance on CLOS).

> I find the name hard to remember in both cases, because that's not
> what comes to my mind when I think of this library (when
> I internet-search for "cl-print", I find mostly shops that print
> business cards and such stuff...)

It would probably make sense to get rid of the "cl-" prefix at some
point, but that will cause `cl-prin1` and `prin1` to collide, so it
would require to merge the two somehow.  We might want to do it down the
road, but I figured it as better to move forward with a separate name
space so we don't have to solve that problem before we can use the
new functionality.

> Anyway, we really really need to define a printer for thunk.el thunks,
> and maybe an extra printer for streams (that's why I CC Nicolas), and
> probably some similar other stuff!

If you look in the cl-print.el I installed into the master branch,
you'll see it already has support for printing advice objects, so it
should be fairly easy to use the same approach to add support for
thunks, streams, iterators, ... (and contrary to what I did for advice,
that support can be placed in thunk.el, stream.el, ...).  Basically, the
question is to figure out how to *recognize* those functions.


        Stefan


PS: I wish we could have funcallable defstructs, so we could
    efficiently dispatch on the "type" of functions when those functions
    are used as objects.

Re: Elisp printer

[Michael Heerdegen]

Hi Stefan,

I noticed that pp.el doesn't handle the #<...> print syntax very well.
I asked myself whether for the purpose of font-lock, indenting, moving
by parens etc - it would make sense to switch to a print syntax that is
`read'able (pseudo) Lisp so that we could just use Emacs-Lisp mode to
present/work with the print results?  I think that could simplify the
work with the new printer.


Michael.

Re: Elisp printer

[Stefan Monnier]

> I noticed that pp.el doesn't handle the #<...> print syntax very well.

Which ones in particular have you noticed (it's used in different ways:
some are very old some are more recent (e.g. new in cl-print), some are
very simple, others more complex, ...)?

> I asked myself whether for the purpose of font-lock, indenting, moving
> by parens etc - it would make sense to switch to a print syntax that is
> `read'able (pseudo) Lisp so that we could just use Emacs-Lisp mode to
> present/work with the print results?  I think that could simplify the
> work with the new printer.

We could use a syntax more like that of structs, i.e. something of the
form #s(...).  For those objects which really aren't structs at all, we
could use a similar notation with another letter (e.g. #f(...) for
function objects such as advice thingies)?


        Stefan

Re: Elisp printer

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > I noticed that pp.el doesn't handle the #<...> print syntax very well.
>
> Which ones in particular have you noticed (it's used in different ways:
> some are very old some are more recent (e.g. new in cl-print), some are
> very simple, others more complex, ...)?

I tried with (symbol-function 'byte-compile-arglist-warn), where I have
advised that function, and got

#+begin_src emacs-lisp
#<advice-wrapper :around #<compiled-function
(name arglist macrop)
#<bytecode> > my-byte-compile-arglist-warn--around-ad>
#+end_src

which not really looks neat (especially with the unindented broken
lines).  I produced that with a hacked `pp-to-string' that uses
`cl-prin1' as printer.

> We could use a syntax more like that of structs, i.e. something of the
> form #s(...).  For those objects which really aren't structs at all, we
> could use a similar notation with another letter (e.g. #f(...) for
> function objects such as advice thingies)?

Is just one letter enough?  How would thunks and streams look like with
this scheme?


Michael.

Re: Elisp printer

[Stefan Monnier]

> I tried with (symbol-function 'byte-compile-arglist-warn), where I have
> advised that function, and got

> #+begin_src emacs-lisp
> #<advice-wrapper :around #<compiled-function
> (name arglist macrop)
> #<bytecode> > my-byte-compile-arglist-warn--around-ad>
> #+end_src

I see, yes, that's ugly.

>> We could use a syntax more like that of structs, i.e. something of the
>> form #s(...).  For those objects which really aren't structs at all, we
>> could use a similar notation with another letter (e.g. #f(...) for
>> function objects such as advice thingies)?
> Is just one letter enough?

Of course: the "#s(..)" is really "#s(<type> ...)" so the above example
could look like

    #f(advice-wrapper :around #f(compiled-function (name arglist macrop)
                                                   #<bytecode>)
                       my-byte-compile-arglist-warn--around-ad)

> How would thunks and streams look like with
> this scheme?

You'd get to choose when you implement their corresponding methods but
they could look like

    #f(thunk ...)
and
    #f(stream ...)


-- Stefan

Re: Elisp printer

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

Stefan> PS: I wish we could have funcallable defstructs, so we could
Stefan>     efficiently dispatch on the "type" of functions when those functions
Stefan>     are used as objects.

I was wondering about this a while ago and thinking that maybe it could
be done without C changes by repurposing the extra arguments to
make-byte-code.  The idea here would be to let cl-defsubst take a new
:type argument other than 'vector or 'list, meaning "use
make-byte-code"; and set the :initial-offset to skip over the stuff
necessary for the bytecode.

It's kind of hacky though.  I think it's probably better to just add
funcallable instances directly, and real types of some kind at the same
time.

Tom

Re: Elisp printer

[Stefan Monnier]

> I was wondering about this a while ago and thinking that maybe it could
> be done without C changes by repurposing the extra arguments to
> make-byte-code.  The idea here would be to let cl-defsubst take a new
> :type argument other than 'vector or 'list, meaning "use
> make-byte-code"; and set the :initial-offset to skip over the stuff
> necessary for the bytecode.

Yes, that can be done fairly easily.  But that doesn't give you callable
functions: it just gives you structs represented by those special
"compiled-function" vectors.

E.g. thunk.el creates its thunks with:

    (defmacro thunk-delay (&rest body)
      "Delay the evaluation of BODY."
      (declare (debug t))
      (let ((forced (make-symbol "forced"))
            (val (make-symbol "val")))
        `(let (,forced ,val)
           (lambda (&optional check)
             (if check
                 ,forced
               (unless ,forced
                 (setf ,val (progn ,@body))
                 (setf ,forced t))
               ,val)))))

so we'd need some way to specify both the function's body and its
"struct" at the same time.  What I was thinking of was something like

    (callable-defstruct thunk
      val forced)

    (defmacro thunk-delay (&rest body)
      "Delay the evaluation of BODY."
      (declare (debug t))
      `(make-thunk (&optional check)
         (if check
             forced
           (unless forced
             (setq val (progn ,@body))
             (setq forced t))
           val)))

The idea would be that `forced` and `val` would be fields of the
"callable-struct" and would be accessible directly from the body of the
function (as well as from the outside via thunk-val and thunk-forced
accessors).  That would require changes to the byte-compiler (mostly in
cconv.el) but it can probably be made to work without significant
changes at the C level.


        Stefan

Re: Elisp printer

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I was wondering about this a while ago and thinking that maybe it could
>> be done without C changes by repurposing the extra arguments to
>> make-byte-code.  The idea here would be to let cl-defsubst take a new
>> :type argument other than 'vector or 'list, meaning "use
>> make-byte-code"; and set the :initial-offset to skip over the stuff
>> necessary for the bytecode.

Stefan> Yes, that can be done fairly easily.  But that doesn't give you
Stefan> callable functions: it just gives you structs represented by
Stefan> those special "compiled-function" vectors.

What I forgot to mention is that this would be coupled with a slot in
the base class that holds the function to "actually call", and the
underlying call to make-byte-code would install some bytecode that would
simply funcall whatever is in this slot, maybe passing in the object as
a first argument.

I'm not 100% sure this addresses what you're looking for; if not I'd
appreciate it if you could explain more.

One thing I note is that this doesn't provide the "sugar" of being able
to refer to slots using their bare name, you'd have to use accessors.
I'm not sure that this omission is so bad, but maybe make-thunk could
wrap the body of the generated function in some cl-symbol-macrolet forms

Tom

Re: Elisp printer

[Stefan Monnier]

> What I forgot to mention is that this would be coupled with a slot in
> the base class that holds the function to "actually call", and the
> underlying call to make-byte-code would install some bytecode that would
> simply funcall whatever is in this slot, maybe passing in the object as
> a first argument.

But that adds a funcall-indirection.  Given the cost of funcalls in
Elisp, this is a fairly major price to pay.  In that case, you're better
off representing your functions as symbols, so you store the actual
function in the function slot, and the data slots in the symbol-plist or
symbol-value slots.

> I'm not 100% sure this addresses what you're looking for; if not I'd
> appreciate it if you could explain more.

Yes, it would, except I don't like its performance.

> One thing I note is that this doesn't provide the "sugar" of being able
> to refer to slots using their bare name, you'd have to use accessors.

That sugar is just an attractive idea, not indispensable.


        Stefan

Re: Elisp printer

[Lars Brinkhoff]

Tom Tromey <tom@tromey.com> writes:
> It's kind of hacky though.  I think it's probably better to just add
> funcallable instances directly, and real types of some kind at the same
> time.

I tried to submit a patch for user-defined record types some years ago.
Instances were pseudovectors, with the first element being a symbol
naming its type.

Re: Elisp printer

[Stefan Monnier]

>> It's kind of hacky though.  I think it's probably better to just add
>> funcallable instances directly, and real types of some kind at the same
>> time.
> I tried to submit a patch for user-defined record types some years ago.
> Instances were pseudovectors, with the first element being a symbol
> naming its type.

Yes, I think we're pretty much ready to accept such a patch,


        Stefan

Re: Elisp printer

[Lars Brinkhoff]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> It's kind of hacky though.  I think it's probably better to just add
>>> funcallable instances directly, and real types of some kind at the same
>>> time.
>> I tried to submit a patch for user-defined record types some years ago.
>> Instances were pseudovectors, with the first element being a symbol
>> naming its type.
> Yes, I think we're pretty much ready to accept such a patch,

Great, I'll see if I can dust if off and send it in again.

New pp (was: bug#25295: 26.0.50; Represent eieio objects using object-print in backtraces and edebug)

[Stefan Monnier]

> I posted an initial draft at
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25122#21, I'm not seeing
> as much speedup as I'd hoped, though I haven't tried optimizing it yet.

Thanks.  I answered there about the performance aspect.

> +(defun pp-prin1 (object &optional stream)
> +  (let ((cl-print-readably nil)
> +        (stream (make-pp-state (or stream standard-output))))
> +    (pp--scan :open-block stream)
> +    (prog1 (cl-prin1 object (cons :pprint stream))
                               ^^^^^^^^^^^^^^^^^^^^^
Why not just pass `stream' since it's a struct and we can hence dispatch
based on its type?

> +;; fallback to standard `cl-print-object'.
> +(cl-defmethod cl-print-object (object (stream (head :pprint)))
> +  (pp--scan (cl-prin1-to-string object) (cdr stream))
> +  object)

Hmm... but if we use such a pseudo-stream here, doesn't it break all
calls to `princ' within other cl-print-object methods (i.e. forcing us
the override pretty much all existing cl-print-object methods with
a pprint-specific one)?


        Stefan

Re: New pp (was: bug#25295: 26.0.50; Represent eieio objects using object-print in backtraces and edebug)

[Noam Postavsky]

On Sat, Mar 11, 2017 at 10:38 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> +(defun pp-prin1 (object &optional stream)
>> +  (let ((cl-print-readably nil)
>> +        (stream (make-pp-state (or stream standard-output))))
>> +    (pp--scan :open-block stream)
>> +    (prog1 (cl-prin1 object (cons :pprint stream))
>                                ^^^^^^^^^^^^^^^^^^^^^
> Why not just pass `stream' since it's a struct and we can hence dispatch
> based on its type?

Oh, hmm, that makes sense. I took a few wrong turns while implementing
this, probably what happened is that when I decided to use (cons
:pprint) I hadn't decided on a struct yet.

>
>> +;; fallback to standard `cl-print-object'.
>> +(cl-defmethod cl-print-object (object (stream (head :pprint)))
>> +  (pp--scan (cl-prin1-to-string object) (cdr stream))
>> +  object)
>
> Hmm... but if we use such a pseudo-stream here, doesn't it break all
> calls to `princ' within other cl-print-object methods (i.e. forcing us
> the override pretty much all existing cl-print-object methods with
> a pprint-specific one)?

It doesn't break anything, AFAIK, it just means that you don't get
control over newlines vs spaces. So we would need pprint-specific
methods for "big" structures only.

Oh wait, did you possibly miss a close paren? Might be clearer this way:

(cl-defmethod cl-print-object (object (pprint-state (head :pprint)))
  (pp--scan (cl-prin1-to-string object)
            (cdr pprint-state))
  object)

Re: New pp

[Stefan Monnier]

>> Hmm... but if we use such a pseudo-stream here, doesn't it break all
>> calls to `princ' within other cl-print-object methods (i.e. forcing us
>> the override pretty much all existing cl-print-object methods with
>> a pprint-specific one)?
> It doesn't break anything, AFAIK, it just means that you don't get
> control over newlines vs spaces.

But from my reading of the code, we may sometimes end up calling `princ'
with a pp-state object, and I expect it won't know what to do with it.

> Oh wait, did you possibly miss a close paren? Might be clearer this way:
> (cl-defmethod cl-print-object (object (pprint-state (head :pprint)))
>   (pp--scan (cl-prin1-to-string object)
>             (cdr pprint-state))
>   object)

No, the problem is when we run for example the

    (cl-defmethod cl-print-object ((object vector) stream)

method (which AFAIK takes precedence over the

   (cl-defmethod cl-print-object (object (pprint-state (head :pprint)))

method, so it will receive a pp-state stream and pass it on to `princ`).


        Stefan

Re: New pp

[Noam Postavsky]

On Sat, Mar 11, 2017 at 11:40 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> No, the problem is when we run for example the
>
>     (cl-defmethod cl-print-object ((object vector) stream)
>
> method (which AFAIK takes precedence over the
>
>    (cl-defmethod cl-print-object (object (pprint-state (head :pprint)))
>
> method, so it will receive a pp-state stream and pass it on to `princ`).

Oh yeah, that is a problem. Is there any way to get a higher precedence?

Re: New pp

[Stefan Monnier]

>> No, the problem is when we run for example the
>>
>> (cl-defmethod cl-print-object ((object vector) stream)
>>
>> method (which AFAIK takes precedence over the
>>
>> (cl-defmethod cl-print-object (object (pprint-state (head :pprint)))
>>
>> method, so it will receive a pp-state stream and pass it on to `princ`).
> Oh yeah, that is a problem. Is there any way to get a higher precedence?

We could change the cl-defgeneric to specify that the second arg comes
before the first when ordering methods with (:argument-precedence-order
stream object).
But it still feels brittle.


        Stefan

Re: New pp

[Noam Postavsky]

On Sat, Mar 11, 2017 at 11:57 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> We could change the cl-defgeneric to specify that the second arg comes
> before the first when ordering methods with (:argument-precedence-order
> stream object).

Works.

> But it still feels brittle.

Any other ideas?

Re: New pp

[Stefan Monnier]

>> We could change the cl-defgeneric to specify that the second arg comes
>> before the first when ordering methods with (:argument-precedence-order
>> stream object).
> Works.
>> But it still feels brittle.
> Any other ideas?

Add cl-princ?
Use a function for `stream`?
Extend the print.c code to accept other kinds of streams?
Don't try to do it all within cl-print-object but use a separate
function on top of it?

The second option should work right now, except you can't easily
dispatch on them.  The problem with it is that it might prove slow (one
function call per character), but maybe we can fix this by changing the
print.c code so those function-valued streams can be called with
a string rather than with a single char.




        Stefan

Re: New pp

[Noam Postavsky]

On Sun, Mar 12, 2017 at 10:06 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> Don't try to do it all within cl-print-object but use a separate
> function on top of it?

Hmm, right, why *did* I try to use cl-print-object? Maybe I had some
vague idea about recursing back into prettyprinting for subelements,
but of course that can't work.

New patch posted at [1] uses pp-print-object instead.

[1]: https://debbugs.gnu.org/cgi/bugreport.cgi?att=1;filename=v2-0001-New-pretty-printer-Bug-25122.patch;msg=60;bug=25122

User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
>> Tom Tromey wrote:
>>> It's kind of hacky though.  I think it's probably better to just add
>>> funcallable instances directly, and real types of some kind at the
>>> same time.
>> I tried to submit a patch for user-defined record types some years
>> ago.  Instances were pseudovectors, with the first element being a
>> symbol naming its type.
> Yes, I think we're pretty much ready to accept such a patch

This is my old patch dusted off and rebased to current master.
It's just a raw material posted for review.

A test case would be:

  (let ((x (make-record 'foo 3 nil)))
    (aset x 1 1)
    (aset x 2 2)
    (aset x 3 3)
    (list (read-from-string (with-output-to-string (prin1 x)))
    (recordp x)
    (type-of x)
    (aref x 0)
    (aref x 3)
    (length x)))

This evalates to ((#%[foo 1 2 3] . 13) t foo foo 3 4).


diff --git a/src/alloc.c b/src/alloc.c
index ae3e151..de08276 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3392,6 +3392,46 @@ struct buffer *
   return b;
 }
 
+static void
+check_record_type (Lisp_Object type)
+{
+  if (!SYMBOLP(type))
+    error ("Invalid type; must be symbol");
+}
+
+static struct Lisp_Vector *
+allocate_record (int count)
+{
+  if (count >= (1 << PSEUDOVECTOR_SIZE_BITS))
+    error ("Record too large");
+
+  struct Lisp_Vector *p = allocate_vector (count);
+  XSETPVECTYPE (p, PVEC_RECORD);
+  return p;
+}
+
+DEFUN ("make-record", Fmake_record, Smake_record, 3, 3, 0,
+       doc: /* Create a new record of type TYPE with SLOTS elements, each initialized to INIT.  */)
+  (Lisp_Object type, Lisp_Object slots, Lisp_Object init)
+{
+  Lisp_Object vector;
+  ptrdiff_t size, i;
+  struct Lisp_Vector *p;
+
+  CHECK_NATNUM (slots);
+  check_record_type (type);
+
+  size = XFASTINT (slots) + 1;
+  p = allocate_record (size);
+  p->contents[0] = type;
+  for (i = 1; i < size; i++)
+    p->contents[i] = init;
+
+  XSETVECTOR (vector, p);
+  return vector;
+}
+
+
 DEFUN ("make-vector", Fmake_vector, Smake_vector, 2, 2, 0,
        doc: /* Return a newly created vector of length LENGTH, with each element being INIT.
 See also the function `vector'.  */)
@@ -7465,6 +7505,7 @@ This means that certain objects should be allocated in shared (pure) space.
   defsubr (&Smake_byte_code);
   defsubr (&Smake_list);
   defsubr (&Smake_vector);
+  defsubr (&Smake_record);
   defsubr (&Smake_string);
   defsubr (&Smake_bool_vector);
   defsubr (&Smake_symbol);
diff --git a/src/data.c b/src/data.c
index ae8dd97..eceb752 100644
--- a/src/data.c
+++ b/src/data.c
@@ -267,6 +267,7 @@ static void swap_in_symval_forwarding (struct Lisp_Symbol *,
         case PVEC_MUTEX: return Qmutex;
         case PVEC_CONDVAR: return Qcondition_variable;
         case PVEC_TERMINAL: return Qterminal;
+        case PVEC_RECORD: return AREF (object, 0);
         /* "Impossible" cases.  */
         case PVEC_XWIDGET:
         case PVEC_OTHER:
@@ -359,6 +360,15 @@ static void swap_in_symval_forwarding (struct Lisp_Symbol *,
   return Qnil;
 }
 
+DEFUN ("recordp", Frecordp_p, Srecordp, 1, 1, 0,
+       doc: /* Return t if OBJECT is a record.  */)
+  (Lisp_Object object)
+{
+  if (RECORDP (object))
+    return Qt;
+  return Qnil;
+}
+
 DEFUN ("stringp", Fstringp, Sstringp, 1, 1, 0,
        doc: /* Return t if OBJECT is a string.  */
        attributes: const)
@@ -2287,7 +2297,7 @@ If the current binding is global (the default), the value is nil.  */)
       ptrdiff_t size = 0;
       if (VECTORP (array))
 	size = ASIZE (array);
-      else if (COMPILEDP (array))
+      else if (COMPILEDP (array) || RECORDP (array))
 	size = ASIZE (array) & PSEUDOVECTOR_SIZE_MASK;
       else
 	wrong_type_argument (Qarrayp, array);
@@ -2308,7 +2318,8 @@ If the current binding is global (the default), the value is nil.  */)
 
   CHECK_NUMBER (idx);
   idxval = XINT (idx);
-  CHECK_ARRAY (array, Qarrayp);
+  if (! RECORDP (array))
+    CHECK_ARRAY (array, Qarrayp);
 
   if (VECTORP (array))
     {
@@ -2328,7 +2339,14 @@ If the current binding is global (the default), the value is nil.  */)
       CHECK_CHARACTER (idx);
       CHAR_TABLE_SET (array, idxval, newelt);
     }
-  else
+  else if (RECORDP (array))
+    {
+      ptrdiff_t size = ASIZE (array) & PSEUDOVECTOR_SIZE_MASK;
+      if (idxval < 0 || idxval >= size)
+	args_out_of_range (array, idx);
+      ASET (array, idxval, newelt);
+    }
+  else /* STRINGP */
     {
       int c;
 
@@ -3714,6 +3732,7 @@ enum bool_vector_op { bool_vector_exclusive_or,
   DEFSYM (Qbuffer, "buffer");
   DEFSYM (Qframe, "frame");
   DEFSYM (Qvector, "vector");
+  DEFSYM (Qrecord, "record");
   DEFSYM (Qchar_table, "char-table");
   DEFSYM (Qbool_vector, "bool-vector");
   DEFSYM (Qhash_table, "hash-table");
@@ -3750,6 +3769,7 @@ enum bool_vector_op { bool_vector_exclusive_or,
   defsubr (&Sstringp);
   defsubr (&Smultibyte_string_p);
   defsubr (&Svectorp);
+  defsubr (&Srecordp);
   defsubr (&Schar_table_p);
   defsubr (&Svector_or_char_table_p);
   defsubr (&Sbool_vector_p);
diff --git a/src/fns.c b/src/fns.c
index 1065355..36bde20 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -104,7 +104,7 @@ static void sort_vector_copy (Lisp_Object, ptrdiff_t,
     XSETFASTINT (val, MAX_CHAR);
   else if (BOOL_VECTOR_P (sequence))
     XSETFASTINT (val, bool_vector_size (sequence));
-  else if (COMPILEDP (sequence))
+  else if (COMPILEDP (sequence) || RECORDP (sequence))
     XSETFASTINT (val, ASIZE (sequence) & PSEUDOVECTOR_SIZE_MASK);
   else if (CONSP (sequence))
     {
diff --git a/src/lisp.h b/src/lisp.h
index ab4db4c..fb5fed1 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -874,6 +874,7 @@ enum pvec_type
   PVEC_TERMINAL,
   PVEC_WINDOW_CONFIGURATION,
   PVEC_SUBR,
+  PVEC_RECORD,
   PVEC_OTHER,            /* Should never be visible to Elisp code.  */
   PVEC_XWIDGET,
   PVEC_XWIDGET_VIEW,
@@ -2728,6 +2729,12 @@ enum char_bits
   return PSEUDOVECTORP (a, PVEC_FRAME);
 }
 
+INLINE bool
+RECORDP (Lisp_Object a)
+{
+  return PSEUDOVECTORP (a, PVEC_RECORD);
+}
+
 /* Test for image (image . spec)  */
 INLINE bool
 IMAGEP (Lisp_Object x)
diff --git a/src/lread.c b/src/lread.c
index 5c6a7f9..1fcbc37 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -2762,6 +2762,19 @@ BUFFER is the buffer to evaluate (nil means use current buffer),
 	  make_byte_code (vec);
 	  return tmp;
 	}
+      if (c == '%')
+	{
+	  c = READCHAR;
+	  if (c == '[')
+	    {
+	      Lisp_Object tmp;
+	      tmp = read_vector (readcharfun, 1);
+	      XSETPVECTYPE (XVECTOR(tmp), PVEC_RECORD);
+	      return tmp;
+	    }
+	  UNREAD (c);
+	  invalid_syntax ("#");
+	}
       if (c == '(')
 	{
 	  Lisp_Object tmp;
diff --git a/src/print.c b/src/print.c
index e857761..f7ecd3c 100644
--- a/src/print.c
+++ b/src/print.c
@@ -1966,6 +1966,7 @@
       case PVEC_SUB_CHAR_TABLE:
       case PVEC_COMPILED:
       case PVEC_CHAR_TABLE:
+      case PVEC_RECORD:
       case PVEC_NORMAL_VECTOR: ;
 	{
 	  ptrdiff_t size = ASIZE (obj);
@@ -1974,6 +1975,12 @@
 	      printchar ('#', printcharfun);
 	      size &= PSEUDOVECTOR_SIZE_MASK;
 	    }
+	  if (RECORDP (obj))
+	    {
+	      printchar ('#', printcharfun);
+	      printchar ('%', printcharfun);
+	      size &= PSEUDOVECTOR_SIZE_MASK;
+	    }
 	  if (CHAR_TABLE_P (obj) || SUB_CHAR_TABLE_P (obj))
 	    {
 	      /* We print a char-table as if it were a vector,

Re: User-defined record types

[Lars Brinkhoff]

Lars Brinkhoff wrote:
> This is my old patch dusted off and rebased to current master.
> It's just a raw material posted for review.

This is how cl-defstruct could be modified to optionally make record
instances.  More work would probably be needed in cl-preloaded.el and
cl-generic.el.

Test case:

  (cl-defstruct (foo (:type record)) x y z)
  (let ((x (make-foo :y 1)))
    (list (type-of x)
          (foo-p x)
          (recordp x)
          (foo-y x)
          x))



diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 40342f3..dead86e 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2544,6 +2544,12 @@ cl--sublis
       (cons (cl--sublis alist (car tree)) (cl--sublis alist (cdr tree))))
      (t tree))))
 
+(defun record (type &rest elements)
+  (let ((result (make-record type (length elements) nil))
+	(i 0))
+    (dolist (elt elements result)
+      (aset result (cl-incf i) elt))))
+
 ;;; Structures.
 
 (defmacro cl--find-class (type)
@@ -2656,6 +2662,8 @@ cl-defstruct
 				  descs)))
 	      (t
 	       (error "Structure option %s unrecognized" opt)))))
+    (if (eq type 'record)
+        (setq named t))
     (unless (or include-name type)
       (setq include-name cl--struct-default-parent))
     (when include-name (setq include (cl--struct-get-class include-name)))
@@ -2684,7 +2692,7 @@ cl-defstruct
 	  (if (cl--struct-class-named include) (setq tag name named t)))
       (if type
 	  (progn
-	    (or (memq type '(vector list))
+	    (or (memq type '(vector list record))
 		(error "Invalid :type specifier: %s" type))
 	    (if named (setq tag name)))
 	(setq named 'true)))
@@ -2700,6 +2708,9 @@ cl-defstruct
                              `(and (vectorp cl-x)
                                    (>= (length cl-x) ,(length descs))
                                    (memq (aref cl-x ,pos) ,tag-symbol)))
+                            ((eq type 'record)
+                             `(and (recordp cl-x)
+                                   (memq (type-of cl-x) ,tag-symbol)))
                             ((= pos 0) `(memq (car-safe cl-x) ,tag-symbol))
                             (t `(and (consp cl-x)
 				     (memq (nth ,pos cl-x) ,tag-symbol))))))
@@ -2740,7 +2751,7 @@ cl-defstruct
 			      (list `(or ,pred-check
                                          (signal 'wrong-type-argument
                                                  (list ',name cl-x)))))
-                       ,(if (memq type '(nil vector)) `(aref cl-x ,pos)
+                       ,(if (memq type '(nil vector record)) `(aref cl-x ,pos)
                           (if (= pos 0) '(car cl-x)
                             `(nth ,pos cl-x))))
                     forms)

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
>> diff --git a/src/data.c b/src/data.c
>> index ae8dd97..eceb752 100644
>> --- a/src/data.c
>> +++ b/src/data.c
>> @@ -267,6 +267,7 @@ static void swap_in_symval_forwarding (struct
>> Lisp_Symbol *,
>>          case PVEC_MUTEX: return Qmutex;
>>          case PVEC_CONDVAR: return Qcondition_variable;
>>          case PVEC_TERMINAL: return Qterminal;
>> +        case PVEC_RECORD: return AREF (object, 0);
>>          /* "Impossible" cases.  */
>>          case PVEC_XWIDGET:
>>          case PVEC_OTHER:
>
> `type-of` is expected to return a symbol.  So the above code implies
> that `record` objects should have a symbol their slot 0 and that this
> symbol should be the record's type name.

Right.  Fmake_record does check that slot 0 is initialized to a symbol.

> Currently EIEIO and `cl-defstruct` indeed puts a symbol in slot 0 but
> this symbol is not exactly the struct type, so returning AREF (object,
> 0) wouldn't return quite the right value.

I don't understand everything about cl-defstruct, and even less about
Emacs flavor of generic functions or EIEIO.  I have a patch which
demostrates how to make cl-defstruct create record instances if you
explicitly ask for it.  It does put the type name in slot 0.

If you pass (:named foo) to cl-defstruct, foo will be put in list or
vector slot 0, so I suppose that should work for records too.

If you don't use the :named option, it seems there will be no type
information in the instances.

Re: User-defined record types

[Lars Brinkhoff]

> If you pass (:named foo) to cl-defstruct, foo will be put in list or
> vector slot 0, so I suppose that should work for records too.
>
> If you don't use the :named option, it seems there will be no type
> information in the instances.

I see now this is wrong, and there are more cases to consider.
This is the situation as of now:

- If you don't pass the :type option to cl-defstruct, you get a
cl-struct-FOO type tag in slot 0.

- If you pass :type but not :named, you don't get any type tag.

- If you pass :type and :named, you get a type tag in slot 0, which is
the same symbol as the struct name.

Re: User-defined record types

[Stefan Monnier]

>> If you pass (:named foo) to cl-defstruct, foo will be put in list or
>> vector slot 0, so I suppose that should work for records too.
>> If you don't use the :named option, it seems there will be no type
>> information in the instances.
> I see now this is wrong, and there are more cases to consider.
> This is the situation as of now:
> - If you don't pass the :type option to cl-defstruct, you get a
> cl-struct-FOO type tag in slot 0.

That's right.  And this symbol's value is the class object.
The same holds for EIEIO.

These are the main candidates for use of the new `record` type.

> - If you pass :type but not :named, you don't get any type tag.

And those should not use `record` but `vector` or `list`
depending on the :type that was specified.

> - If you pass :type and :named, you get a type tag in slot 0, which is
> the same symbol as the struct name.

But again, these shouldn't use `record`.


        Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> - If you don't pass the :type option to cl-defstruct, you get a
>> cl-struct-FOO type tag in slot 0.
>
> That's right.  And this symbol's value is the class object.
> The same holds for EIEIO.
>
> These are the main candidates for use of the new `record` type.

Yes, at first I tried to make cl-deftype do this by default.  But I ran
into problems in cl-preloaded.el and cl-generic.el.

>> - If you pass :type but not :named, you don't get any type tag.
>
> And those should not use `record` but `vector` or `list`
> depending on the :type that was specified.
>
>> - If you pass :type and :named, you get a type tag in slot 0, which is
>> the same symbol as the struct name.
>
> But again, these shouldn't use `record`.

Right, I just wanted to list all possible variations to gain some
understanding.  For example, I don't know if it makes a difference if
the slot 0 type tag has the form cl-struct-FOO as is the default, or
just FOO as when you specify :type.  But now I know there might be.

Re: User-defined record types

[Stefan Monnier]

>>> - If you don't pass the :type option to cl-defstruct, you get a
>>> cl-struct-FOO type tag in slot 0.
>> That's right.  And this symbol's value is the class object.
>> The same holds for EIEIO.
>> These are the main candidates for use of the new `record` type.
> Yes, at first I tried to make cl-deftype do this by default.  But I ran
> into problems in cl-preloaded.el and cl-generic.el.

I can help with that.

> Right, I just wanted to list all possible variations to gain some
> understanding.  For example, I don't know if it makes a difference if
> the slot 0 type tag has the form cl-struct-FOO as is the default, or
> just FOO as when you specify :type.  But now I know there might be.

The cl-struct-FOO symbol was used instead of just FOO in order to try
and avoid false-positives (i.e. to try and avoid mistakenly recognizing
some random array as being a cl-struct object).

With `record` you don't need that trick any more, so records could have
the FOO symbol in their slot 0.

For EIEIO objects, it would slow things down a bit, because EIEIO
accessors needs to consult the class object, so the time it takes to get
from (aref obj 0) to the actual class object is important: if we put the
actual type symbol, that means that we'd need something like

    (get (aref obj 0) 'cl--class)

whereas the current code uses

    (symbol-value (aref obj 0))

The problem being that the identifier used for the type name might
already be used as a variable as well as as function, so we can't use
the `symbol-value` or `symbol-function` slot.

As mentioned, ideally, we'd want to store the class object directly in
the slot 0.  The downside is that prin1 would then dump the class object
as well, so when reading dumped objects we'd end up creating another
copy of the class object rather than reusing an existing class object.
And this will break cl-generic dispatch which compares class objects
with `eq`.


        Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
>> Yes, at first I tried to make cl-deftype do this by default.  But I
>> ran into problems in cl-preloaded.el and cl-generic.el.
> I can help with that.

Thanks.  I got past that and made it through to a dumped emacs which
uses record types for defstruct by default.

However, there is still EIEIO which may well need some expert guidance
to update.

> As mentioned, ideally, we'd want to store the class object directly in
> the slot 0.  The downside is that prin1 would then dump the class object
> as well, so when reading dumped objects we'd end up creating another
> copy of the class object rather than reusing an existing class object.
> And this will break cl-generic dispatch which compares class objects
> with `eq`.

Understood.  I'll look into storing the class object in slot 0.  I
imagine there will be some circular bootstrapping problem, e.g. creating
the first record object requires a class object which is a record
object.

Some suggestions:

- type-of looks into the class object and returns the symbol naming the
  class.

- class-of could be introduced to return a class object, like CLOS.

- The read/print syntax for records uses the symbol for the type slot.
  Reading a record would then maybe only work right if the class object
  has been defined first.  Not sure if that's ok.

Re: User-defined record types

[Stefan Monnier]

> Thanks.  I got past that and made it through to a dumped emacs which
> uses record types for defstruct by default.

Cool.

> However, there is still EIEIO which may well need some expert guidance
> to update.

I can take care of that.  Can you put your code on a branch like
scratch/record?

>> As mentioned, ideally, we'd want to store the class object directly in
>> the slot 0.  The downside is that prin1 would then dump the class object
>> as well, so when reading dumped objects we'd end up creating another
>> copy of the class object rather than reusing an existing class object.
>> And this will break cl-generic dispatch which compares class objects
>> with `eq`.
> Understood.  I'll look into storing the class object in slot 0.
> I imagine there will be some circular bootstrapping problem, e.g. creating
> the first record object requires a class object which is a record
> object.

Yes, cl-preloaded.el would likely need some extra care to get the system
to bootstrap, but I'm not worried about that.

> Some suggestions:
> - type-of looks into the class object and returns the symbol naming the
>   class.

Yes, that would make a lot of sense, but that means we have to impose
a particular shape on the contents of slot 0.  IOW it means that the
representation of classes is at least somewhat known/imposed by the
C code.  It's probably OK.

> - class-of could be introduced to return a class object, like CLOS.

Sure.

> - The read/print syntax for records uses the symbol for the type slot.
>   Reading a record would then maybe only work right if the class object
>   has been defined first.  Not sure if that's ok.

Not sure either.  Of course, another option is for prin1 to print the
whole class object in slot 0, and when we read it in, we handle it
specially by looking at the class name inside the class object and
reusing the corresponding class if it already exists.  The would likely
hard-code even more of the notion of class-objects into the C code.


        Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
>> However, there is still EIEIO which may well need some expert guidance
>> to update.
>
> I can take care of that.  Can you put your code on a branch like
> scratch/record?

Certainly.  Do I need to apply for project membership or write
permission or something like that?

Re: User-defined record types

[Stefan Monnier]

> Certainly.  Do I need to apply for project membership or write
> permission or something like that?

If you don't have write access yet, then yes.  Just go to your Savannah
account and request membership in the `emacs` group.


        Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
>> However, there is still EIEIO which may well need some expert guidance
>> to update.
> I can take care of that.  Can you put your code on a branch like
> scratch/record?

Done, scratch/record it is.

Note that this doesn't survive a full build due to EIEIO breakage.
I will usually build from master, then checkout the branch and rebuild
the changed files.

Re: User-defined record types

[Stefan Monnier]

> Note that this doesn't survive a full build due to EIEIO breakage.

I'll see about fixing this, thanks,


        Stefan

Re: User-defined record types

[Stefan Monnier]

>> However, there is still EIEIO which may well need some expert guidance
>> to update.
> I can take care of that.  Can you put your code on a branch like
> scratch/record?

OK, it's not working.
I found a problem with cl-defstruct objects, tho:

We're going to have trouble preserving backward compatibility with
pre-existing .elc files.  If you look at the macro expansion of current
cl-defstruct:

    (macroexpand '(cl-defstruct (sm-test) a b))
  =>
    (progn
      (defvar cl-struct-sm-test-tags)
      (define-inline sm-test-p (x) (declare (side-effect-free error-free))
        (inline-letevals (x)
                         (inline-quote
                          (and (vectorp #1=,x) (>= (length #1#) 3)
                               (memq (aref #1# 0) cl-struct-sm-test-tags)
                               t))))
      (put 'sm-test 'cl-deftype-satisfies 'sm-test-p)
      (define-inline sm-test-a #2=(x)
        "Access slot \"a\" of `(sm-test)' struct CL-X."
        #3=(declare (side-effect-free t))
        (inline-letevals #4=(x)
          (inline-quote
           (progn
             (or (memq (aref #5=,x 0) cl-struct-sm-test-tags)
                 (signal #6='wrong-type-argument (list 'sm-test . #7=(,x))))
             (aref #8=,x 1)))))
      (define-inline sm-test-b #2# "Access slot \"b\" of `(sm-test)' struct CL-X." #3#
        (inline-letevals #4#
          (inline-quote
           (progn
             (or (memq (aref #5# 0) cl-struct-sm-test-tags)
                 (signal #6# (list 'sm-test . #7#)))
             (aref #8# 2)))))
      (defalias 'copy-sm-test (function copy-sequence))
      (cl-defsubst make-sm-test
          (&cl-defs (nil . #9=((cl-tag-slot) (a) (b))) &key a b)
        "Constructor for objects of type `sm-test'."
        (declare (side-effect-free t))
        (vector 'cl-struct-sm-test a b))
      (eval-and-compile
        (cl-struct-define 'sm-test nil 'cl-structure-object 'nil nil '#9# 'cl-struct-sm-test-tags 'cl-struct-sm-test 't))
      'sm-test)

As you can see, the code says it inherits from `cl-structure-object'
(which now is of `record` type), but its constructor `make-sm-test`
creates a vector rather than a record, so (cl-typep 'cl-structure-object)
will fail if it only considers `type-of'.

Even if we could arrange for cl-struct-define to override the
definition of make-sm-test, that wouldn't help with all the places
where make-sm-test has been inlined already.

[ Of course, the reverse could happen as well: a new struct type (using
  `record` in its constructor) could inherit from a parent that's still
  using `vector`.  But I think we can arrange for that to happen only in
  cases we don't care to support.  ]


        Stefan

Re: User-defined record types

[Stefan Monnier]

>>> However, there is still EIEIO which may well need some expert guidance
>>> to update.
>> I can take care of that.  Can you put your code on a branch like
>> scratch/record?
> OK, it's not working.
           ^^^
           now
Duh!


        Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
> OK, it's [now] working.

A full bootstrap build still fails for me:

cedet/ede.el:46:1:Error: Wrong type argument: sequencep, #%[cl-slot-descriptor expanded nil boolean ((:documentation . "State of an object being expanded in speedbar."))]

> We're going to have trouble preserving backward compatibility with
> pre-existing .elc files.  As you can see, the code says it inherits
> from `cl-structure-object' (which now is of `record` type), but its
> constructor `make-sm-test` creates a vector rather than a record, so
> (cl-typep 'cl-structure-object) will fail if it only considers
> `type-of'.

What if we relax the type check to use (aref object 0) instead?  That
works for both old and new instances, as long as :initial-offset is 0.

Re: User-defined record types

[Stefan Monnier]

> A full bootstrap build still fails for me:
> cedet/ede.el:46:1:Error: Wrong type argument: sequencep,
> #%[cl-slot-descriptor expanded nil boolean ((:documentation . "State of an
> object being expanded in speedbar."))]

Looks like I missed a place where a copy-sequence needs to be turned
into a copy-record?  I know there's still this bug in cl-defstruct's
definition of the copy-<foo> function, but I think this is never used,
so it's probably somewhere else.

>> We're going to have trouble preserving backward compatibility with
>> pre-existing .elc files.  As you can see, the code says it inherits
>> from `cl-structure-object' (which now is of `record` type), but its
>> constructor `make-sm-test` creates a vector rather than a record, so
>> (cl-typep 'cl-structure-object) will fail if it only considers
>> `type-of'.
> What if we relax the type check to use (aref object 0) instead?

But we need an arrayp or recordp check before, so it'd be something like

   (and (or (arrayp x) (recordp x)) (aref x 0))

Problem is that now we're slower than we were before the introduction of
`record`.  One option could be to change `type-of' so as to know about
the old struct format, and to make this backward compatibility hack
dependent on a boolean var which defaults to nil but would be set to
t as soon as we detect the use of an old-style struct.

> That works for both old and new instances, as long as :initial-offset
> is 0.

:initial-offset is always 0 if :type is not `vector` or `list`, so we
don't have to worry about this issue.


        Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
> Looks like I missed a place where a copy-sequence needs to be turned
> into a copy-record?  I know there's still this bug in cl-defstruct's
> definition of the copy-<foo> function, but I think this is never used,
> so it's probably somewhere else.

I fixed that, but it was probably the one in EIEIO's clone method.

Full bootstrap works now.

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
>> What if we relax the type check to use (aref object 0) instead?
>
> One option could be to change `type-of' so as to know about the old
> struct format, and to make this backward compatibility hack dependent
> on a boolean var which defaults to nil but would be set to t as soon
> as we detect the use of an old-style struct.

I have now implemented that in the latest commit to the scratch/record
branch.

Re: User-defined record types

[Stefan Monnier]

We could auto-detect the use of old-style structs (and set the
compatibility var) when cl-struct-define is called with a nil value of
the `type` argument.

[ Of course, this means we need to use a different value for the new
  `record` format.  ]


        Stefan

Re: User-defined record types

[Stefan Monnier]

One more thing: what's up with "record2"?  Which branch should I pay
attention to?  If you don't intend to keep using scratch/record, then
it's better to just replace scratch/record with scratch/record2: you can
rebase/rewrite those `scratch` branches at will.


        Stefan


>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

> We could auto-detect the use of old-style structs (and set the
> compatibility var) when cl-struct-define is called with a nil value of
> the `type` argument.

> [ Of course, this means we need to use a different value for the new
>   `record` format.  ]


>         Stefan

Re: User-defined record types

[Lars Brinkhoff]

Stefan Monnier wrote:
> We could auto-detect the use of old-style structs (and set the
> compatibility var) when cl-struct-define is called with a nil value of
> the `type` argument.

Great, I'll add that.

> Of course, this means we need to use a different value for the new
> `record` format.

The symbol `record' springs to mind.

> One more thing: what's up with "record2"?  Which branch should I pay
> attention to?  If you don't intend to keep using scratch/record, then
> it's better to just replace scratch/record with scratch/record2: you
> can rebase/rewrite those `scratch` branches at will.

I'm happy to do that.  I was not sure what the policy is with replaced
branched.  But if that is ok, I'll mercilessly rewrite it until it's
readly for merging.  I have deleted record2 and updated scratch/record.

Re: User-defined record types

[Stefan Monnier]

>> Of course, this means we need to use a different value for the new
>> `record` format.
> The symbol `record' springs to mind.

I'd prefer using a symbol like `blue-sky`, to avoid confusion.

> I'm happy to do that.  I was not sure what the policy is with replaced
> branched.  But if that is ok, I'll mercilessly rewrite it until it's
> readly for merging.  I have deleted record2 and updated scratch/record.

Great, thanks,


        Stefan