Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Stefan Monnier]

> -DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
> +DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
> +       "(if (< (length mark-ring) 2)\
> +	    (error \"Other region must be marked before transposing two regions\")\
> +	  (let* ((num (if current-prefix-arg\
> +			 (prefix-numeric-value current-prefix-arg)\
> +			0))\
> +		 (ring-length (length mark-ring))\
> +		 (eltnum (mod num ring-length))\
> +		 (eltnum2 (mod (1+ num) ring-length)))\
> +	    (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2))))",

Am I the only one who dislikes seeing such Elisp code hidden within our
C files?


        Stefan

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Charles A. Roelli]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Sun, 11 Mar 2018 12:05:11 -0400
> 
> > -DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
> > +DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
> > +       "(if (< (length mark-ring) 2)\
> > +	    (error \"Other region must be marked before transposing two regions\")\
> > +	  (let* ((num (if current-prefix-arg\
> > +			 (prefix-numeric-value current-prefix-arg)\
> > +			0))\
> > +		 (ring-length (length mark-ring))\
> > +		 (eltnum (mod num ring-length))\
> > +		 (eltnum2 (mod (1+ num) ring-length)))\
> > +	    (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2))))",
> 
> Am I the only one who dislikes seeing such Elisp code hidden within our
> C files?

It's not great, no.  (If anybody wants to move it to Lisp, feel free.)

As a tangent, it would be interesting to automatically activate Emacs
Lisp mode in the interactive argument of the DEFUN macro (if that is
possible), as a trial for the "multi major mode" support that was
recently brought up.  Even if that does seem a bit contradictory
w.r.t. what was expressed above. ;)

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Leo Liu]

On 2018-03-11 18:23 +0100, Charles A. Roelli wrote:
[snipped 16 lines]
>> Am I the only one who dislikes seeing such Elisp code hidden within our
>> C files?
>
> It's not great, no.  (If anybody wants to move it to Lisp, feel free.)

I also find it very ugly.

Leo

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Sun, 11 Mar 2018 12:05:11 -0400
> Cc: "Charles A. Roelli" <charles@aurox.ch>
> 
> > -DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
> > +DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
> > +       "(if (< (length mark-ring) 2)\
> > +	    (error \"Other region must be marked before transposing two regions\")\
> > +	  (let* ((num (if current-prefix-arg\
> > +			 (prefix-numeric-value current-prefix-arg)\
> > +			0))\
> > +		 (ring-length (length mark-ring))\
> > +		 (eltnum (mod num ring-length))\
> > +		 (eltnum2 (mod (1+ num) ring-length)))\
> > +	    (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2))))",
> 
> Am I the only one who dislikes seeing such Elisp code hidden within our
> C files?

I have no problems with that.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On 11 March 2018 at 18:13, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> > Date: Sun, 11 Mar 2018 12:05:11 -0400
> > Cc: "Charles A. Roelli" <charles@aurox.ch>
> >
> > > -DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions,
> 4, 5, 0,
> > > +DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions,
> 4, 5,
> > > +       "(if (< (length mark-ring) 2)\
> > > +       (error \"Other region must be marked before transposing two
> regions\")\
> > > +     (let* ((num (if current-prefix-arg\
> > > +                    (prefix-numeric-value current-prefix-arg)\
> > > +                   0))\
> > > +            (ring-length (length mark-ring))\
> > > +            (eltnum (mod num ring-length))\
> > > +            (eltnum2 (mod (1+ num) ring-length)))\
> > > +       (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring
> eltnum2))))",
> >
> > Am I the only one who dislikes seeing such Elisp code hidden within our
> > C files?
>
> I have no problems with that.
>
>
I agree with Eli, it seems fine to me, but I'm not sure what the
alternative is. If it's to move the body of the interactive spec into
a function in an elisp file, and replace the spec with a call to that
function, it seems overly complicated. (It also feel like a layering
violation, but that can only be true if there is a rule or guideline
saying so and I don't know of any.)

[-- Attachment #2: Type: text/html, Size: 2216 bytes --]

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Karl Fogel]

Richard Copley <rcopley@gmail.com> writes:
>I agree with Eli, it seems fine to me, but I'm not sure what the
>alternative is. If it's to move the body of the interactive spec into
>a function in an elisp file, and replace the spec with a call to that
>function, it seems overly complicated. (It also feel like a layering
>violation, but that can only be true if there is a rule or guideline
>saying so and I don't know of any.)

One solution would be to rename the C-defined function to Ftranspose_regions_internal (with no interactive spec), and move the entry point `transpose-regions' out to Lisp with the same interactive spec.  The Lisp function would just invoke `transpose-regions-internal'.

I'd be happy to do this, if there's consensus that it's a good solution.  

(I wrote the original non-interactive Ftranspose_regions, so am perhaps motivated by nostalgia.  Interestingly, while checking my memory to make sure, I discovered that although src/ChangeLog.4 has the correct record for 1994-04-29, in the git history that change is recorded as being authored by RMS -- commit b229b8d187a.  This suggests that there was some information lossage in a VC-system conversion somewhere along the way.  I don't know if we knew that already or not.)

I looked for all other C-defined functions that have a Lisp expression starting with "(" for their interactive spec in the DEFUN, i.e., are not just using the standard code-letter strings for `interactive'.  There appear to be a very small number of such functions:

  - Finsert_char
  - Ftranspose_regions
  - Frename_buffer
  - Fupcase_region
  - Fdowncase_region
  - Fdelete_file
  - Fset_file_modes

Of these, Ftranspose_regions is certainly the most complex, with Fdelete_file and Frename_buffer as runners up.  The others are relatively simple.  Maybe it's fine to just rely on our collective aesthetic judgement to say that the one in Ftranspose_regions is too complex to be tolerated, while the others are okay.

Best regards,
-Karl

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Karl Fogel]

>Richard Copley <rcopley@gmail.com> writes:
>> I agree with Eli, it seems fine to me, but I'm not sure what the
>> alternative is. If it's to move the body of the interactive spec into
>> a function in an elisp file, and replace the spec with a call to that
>> function, it seems overly complicated. (It also feel like a layering
>> violation, but that can only be true if there is a rule or guideline
>> saying so and I don't know of any.)
> 
> One solution would be to rename the C-defined function to
> Ftranspose_regions_internal (with no interactive spec), and move the
> entry point `transpose-regions' out to Lisp with the same interactive
> spec.  The Lisp function would just invoke `transpose-regions-internal'.
> 
> I'd be happy to do this, if there's consensus that it's a good solution.

Some more follow-up on this:

Richard sent an off-list reply (I didn't realize it was off-list until later) saying that the above looked like a good solution.  So I made the change (commit 3a3aa0e05), then reverted (commit 6d2e8fdd) after Eli reiterated that he didn't see a problem with the original code.  I mistakenly thought we had consensus on the change, but clearly I was wrong and further discussion is needed.

So, here is why I would prefer to see that interactive spec moved to Lisp:

Having it in C makes it less hackable.  It's a rather complex interactive spec, and I can imagine someone wanting to play around with it, perhaps with the goal of suggesting a change to the interactive behavior of `transpose-regions'.  If it were in an Elisp file, it would be much easier for people to make those experiments.  (Yes, technically one could find ways to do it anyway, but it would be harder, and in general I think reducing barriers to hackability is a Good Thing.)

Originally I thought my concerns were aesthetic, but when I thought about it more carefully, my concern is really about ease of hacking.  The same argument could apply to the other non-trivial interactive specs defined as `eval'-able strings in C (there are not many of them -- I listed them in my previous mail, quoted below).  To keep things simple, my proposal right now is just about `transpose-regions', though.

If other people have arguments for or against restoring commit 3a3aa0e05, please speak up.  I'll wait at least a week before coming to any conclusions about consensus, to ensure there's time for discussion.

Best regards,
-Karl

> (I wrote the original non-interactive Ftranspose_regions, so am perhaps
> motivated by nostalgia.  Interestingly, while checking my memory to make
> sure, I discovered that although src/ChangeLog.4 has the correct record
> for 1994-04-29, in the git history that change is recorded as being
> authored by RMS -- commit b229b8d187a.  This suggests that there was
> some information lossage in a VC-system conversion somewhere along the
> way.  I don't know if we knew that already or not.)
> 
> I looked for all other C-defined functions that have a Lisp expression
> starting with "(" for their interactive spec in the DEFUN, i.e., are not
> just using the standard code-letter strings for `interactive'.  There
> appear to be a very small number of such functions:
> 
>   - Finsert_char
>   - Ftranspose_regions
>   - Frename_buffer
>   - Fupcase_region
>   - Fdowncase_region
>   - Fdelete_file
>   - Fset_file_modes
> 
> Of these, Ftranspose_regions is certainly the most complex, with
> Fdelete_file and Frename_buffer as runners up.  The others are
> relatively simple.  Maybe it's fine to just rely on our collective
> aesthetic judgement to say that the one in Ftranspose_regions is too
> complex to be tolerated, while the others are okay.
> 
> Best regards,
> -Karl

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On 20 March 2018 at 21:33, Karl Fogel <kfogel@red-bean.com> wrote:

>
> Richard sent an off-list reply (I didn't realize it was off-list until
> later) saying that the above looked like a good solution.  So I made the
> change (commit 3a3aa0e05), then reverted (commit 6d2e8fdd) after Eli
> reiterated that he didn't see a problem with the original code.  I
> mistakenly thought we had consensus on the change, but clearly I was wrong
> and further discussion is needed.
>

Yes, sorry, I intended to reply to everyone. I felt I should reply, but
only on my own behalf. I was a bit surprised by your reaction!
:)

[-- Attachment #2: Type: text/html, Size: 951 bytes --]

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Tue, 20 Mar 2018 16:33:52 -0500
> 
> So, here is why I would prefer to see that interactive spec moved to Lisp:
> 
> Having it in C makes it less hackable.  It's a rather complex interactive spec, and I can imagine someone wanting to play around with it, perhaps with the goal of suggesting a change to the interactive behavior of `transpose-regions'.  If it were in an Elisp file, it would be much easier for people to make those experiments.  (Yes, technically one could find ways to do it anyway, but it would be harder, and in general I think reducing barriers to hackability is a Good Thing.)
> 
> Originally I thought my concerns were aesthetic, but when I thought about it more carefully, my concern is really about ease of hacking.  The same argument could apply to the other non-trivial interactive specs defined as `eval'-able strings in C (there are not many of them -- I listed them in my previous mail, quoted below).  To keep things simple, my proposal right now is just about `transpose-regions', though.
> 
> If other people have arguments for or against restoring commit 3a3aa0e05, please speak up.  I'll wait at least a week before coming to any conclusions about consensus, to ensure there's time for discussion.

Thanks.

Here are some reasons why not to make the change:

  . there's nothing wrong with that code
  . having it in C doesn't make hacking harder because one can always
    override functions in Emacs, and moving the interactive spec to
    Lisp will still leave the code preloaded anyway
  . it's good to have a small number of examples of using this feature
    of DEFUN in our sources, so that people knew it's possible, and
    could learn from these examples
  . moving code between files makes forensics harder ("git log -L" and
    its ilk become almost useless, for example)
  . there's nothing wrong with that code

Yes, they are all weak reasons, but so are the proposed reasons in
favor of the change.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>Here are some reasons why not to make the change:

I'll respond point-by-point below, but for the record, I don't feel strongly enough about this to continue arguing for the change if consensus is not imminent.  So if I don't convince you here, Eli, the code will just stay as it is, which is fine.  This was never a very important change to begin with.

>  . there's nothing wrong with that code

A circular argument? :-)  (Well, this point may have been tongue-in-cheek humor on your part.)

>  . having it in C doesn't make hacking harder because one can always
>    override functions in Emacs, and moving the interactive spec to
>    Lisp will still leave the code preloaded anyway

"Is hackable" != "is as easily hackable as some other way".  There are plenty of Elisp programmers who will just stop at the C boundary -- they'll just choose not to hack on that particular interactive spec, if they see it's more complicated than opening up an Elisp file, editing, and hitting M-C-x.  Yes, of course it's "hackable" where it is now; so is everything in Emacs.  I'm just saying that it's more work to hack on than it would be if it were in Elisp.  This seems objectively true, just in the sense of measuring sheer number of keystrokes, never mind even the difference in mental burden.  If you think that "having it in C doesn't make hacking harder" [for some people, anyway], I probably can't persuade you.

>  . it's good to have a small number of examples of using this feature
>    of DEFUN in our sources, so that people knew it's possible, and
>    could learn from these examples

We already have several other examples in that file (I listed them in one of my posts).

>  . moving code between files makes forensics harder ("git log -L" and
>    its ilk become almost useless, for example)

True.  (I already interfered with git log -L for Ftranspose_regions -- sorry about that.)

>  . there's nothing wrong with that code

Naturally I resist the temptation to copy my earlier statement!

>Yes, they are all weak reasons, but so are the proposed reasons in
>favor of the change.

Yes, agreed on both counts.

Best regards,
-Karl

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Thu, 22 Mar 2018 00:30:24 -0500
> 
> I'll respond point-by-point below, but for the record, I don't feel strongly enough about this to continue arguing for the change if consensus is not imminent.  So if I don't convince you here, Eli, the code will just stay as it is, which is fine.  This was never a very important change to begin with.

You don't have to convince me.  We are looking for some kind of
consensus here, so if enough people will speak up in support of your
suggestion, we are done.  I posted my reasons to give others some food
for thought, that's all.

Unnecessarily moving stiff between files considered harmful (Was: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive) (Bug#30343)

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Thu, 22 Mar 2018 00:30:24 -0500
> 
> >  . moving code between files makes forensics harder ("git log -L" and
> >    its ilk become almost useless, for example)
> 
> True.

Btw, if I'm allowed a gripe: we have too much of this in our history
already.  Just recently I wanted to know when the assertion in
set_blv_found was born, and was unable to determine this using Git,
because that function was first a macro, then an inline function, and
migrated between 2 or 3 files several times.  I eventually had to use
ChangeLog files from old Emacs releases(!) to find out the history of
this single line.  I'm sure Git gurus will come up with some
convoluted way to do that with Git, and Magit gurus will tell that
Magit already knows how to do this.  But the fact that the usual tools
of trade fail here is for me a clear indication that we should keep
this disadvantage in mind when making such changes: there's a non-zero
price here.

Re: Unnecessarily moving stiff between files considered harmful

[Karl Fogel]

Eli Zaretskii <eliz@gnu.org> writes:
>Btw, if I'm allowed a gripe: we have too much of this in our history
>already.  Just recently I wanted to know when the assertion in
>set_blv_found was born, and was unable to determine this using Git,
>because that function was first a macro, then an inline function, and
>migrated between 2 or 3 files several times.  I eventually had to use
>ChangeLog files from old Emacs releases(!) to find out the history of
>this single line.  I'm sure Git gurus will come up with some
>convoluted way to do that with Git, and Magit gurus will tell that
>Magit already knows how to do this.  But the fact that the usual tools
>of trade fail here is for me a clear indication that we should keep
>this disadvantage in mind when making such changes: there's a non-zero
>price here.

I looked at places in CONTRIBUTE where it might work to add a sentence or two about this, but didn't see any good places to fit it in, hmm.

I agree, there is a price.  (In the case of particular change we were discussing, I thought the price was worth paying, but there are always going to be edge-case disagreements about the relative value of moving code vs the negative effects of the churn.)

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Stefan Monnier]

I haven't really participated so far since it's definitely a bikeshed's
color kind of discussion, but since I'm to blame for it, I'll try and
explain why I brought it up:

I said I "dislike seeing" such code, without any further explanation
because indeed, there's no strong technical reason behind it (I could
come up with various technical reasons, but they'd just be
rationalizations).  I guess it just reflects my "strong types"
background, where I like my code to be sanity checked by a static
analyzer.  For Elisp we don't have any strong static analyzer available,
so we use the next best thing: our byte-compiler's warnings.

With "Elisp embedded in C strings", we don't have any such tool
support, so it makes me feel "unprotected" (we can't even be sure that
parentheses are balanced without run-time testing).

I didn't bring it up with the aim to try and change that specific chunk
of code, but rather to get the opinion of others, and probably also to
try and influence future code.

>> Yes, they are all weak reasons, but so are the proposed reasons in
>> favor of the change.
> Yes, agreed on both counts.

Clearly.


        Stefan

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Karl Fogel]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>I haven't really participated so far since it's definitely a bikeshed's
>color kind of discussion, but since I'm to blame for it, I'll try and
>explain why I brought it up:
>
>I said I "dislike seeing" such code, without any further explanation
>because indeed, there's no strong technical reason behind it (I could
>come up with various technical reasons, but they'd just be
>rationalizations).  I guess it just reflects my "strong types"
>background, where I like my code to be sanity checked by a static
>analyzer.  For Elisp we don't have any strong static analyzer available,
>so we use the next best thing: our byte-compiler's warnings.
>
>With "Elisp embedded in C strings", we don't have any such tool
>support, so it makes me feel "unprotected" (we can't even be sure that
>parentheses are balanced without run-time testing).
>
>I didn't bring it up with the aim to try and change that specific chunk
>of code, but rather to get the opinion of others, and probably also to
>try and influence future code.

I'm not going to push to reinstate the change -- it's not important enough.  But since you're soliciting opinions, I agree with your reasons above, as well as the ones I gave earlier.

However... http://fuchsia.bikeshed.com/, as you said.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Charles A. Roelli]

> Date: Wed, 21 Mar 2018 09:22:19 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Here are some reasons why not to make the change:
> 
>   . there's nothing wrong with that code
>   . having it in C doesn't make hacking harder because one can always
>     override functions in Emacs, 

Good point.  Is this written in the Elisp manual anywhere?  I searched
for "override functions" and "override primitives" in the index, but
did not find a result.  We could add a note saying that functions
defined from C can be overridden from Lisp (possibly in (elisp) What
Is a Function).

> 				   and moving the interactive spec to
>     Lisp will still leave the code preloaded anyway

Instead of defining transpose-regions (and its interactive spec) in
Lisp (as the reverted patch did), we could take the current
interactive spec of transpose-regions and use that to make a preloaded
function named "read-two-regions" in simple.el.  That would allow its
reuse by other commands, too.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Eli Zaretskii]

> Date: Sun, 25 Mar 2018 12:03:35 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> CC: kfogel@red-bean.com, emacs-devel@gnu.org
> 
> > Date: Wed, 21 Mar 2018 09:22:19 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > Here are some reasons why not to make the change:
> > 
> >   . there's nothing wrong with that code
> >   . having it in C doesn't make hacking harder because one can always
> >     override functions in Emacs, 
> 
> Good point.  Is this written in the Elisp manual anywhere?

Yes, it's documented where we describe 'defun':

     Be careful not to redefine existing functions unintentionally.
     ‘defun’ redefines even primitive functions such as ‘car’ without
     any hesitation or notification.  Emacs does not prevent you from
     doing this, because redefining a function is sometimes done
     deliberately, and there is no way to distinguish deliberate
     redefinition from unintentional redefinition.

> I searched for "override functions" and "override primitives" in the
> index, but did not find a result.

I would index this as "redefining functions", at least in addition to
"overriding", if not instead of it.

> Instead of defining transpose-regions (and its interactive spec) in
> Lisp (as the reverted patch did), we could take the current
> interactive spec of transpose-regions and use that to make a preloaded
> function named "read-two-regions" in simple.el.  That would allow its
> reuse by other commands, too.

Where would such a function be useful?

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Charles A. Roelli]

> Date: Sun, 25 Mar 2018 18:41:47 +0300
> From: Eli Zaretskii <eliz@gnu.org>
>
> > >   . having it in C doesn't make hacking harder because one can always
> > >     override functions in Emacs, 
> > 
> > Good point.  Is this written in the Elisp manual anywhere?
> 
> Yes, it's documented where we describe 'defun':
> 
>      Be careful not to redefine existing functions unintentionally.
>      ‘defun’ redefines even primitive functions such as ‘car’ without
>      any hesitation or notification.  Emacs does not prevent you from
>      doing this, because redefining a function is sometimes done
>      deliberately, and there is no way to distinguish deliberate
>      redefinition from unintentional redefinition.

Thanks, this seems correct.
 
> > I searched for "override functions" and "override primitives" in the
> > index, but did not find a result.
> 
> I would index this as "redefining functions", at least in addition to
> "overriding", if not instead of it.

I think both are good, although an index entry for "overriding
functions" could also point to the "advice" documentation, since
"overriding" is a less precise term than "redefining".
 
> > Instead of defining transpose-regions (and its interactive spec) in
> > Lisp (as the reverted patch did), we could take the current
> > interactive spec of transpose-regions and use that to make a preloaded
> > function named "read-two-regions" in simple.el.  That would allow its
> > reuse by other commands, too.
> 
> Where would such a function be useful?

Nothing specific comes to mind.  There is one pair of commands that
already reads multiple regions: ediff-regions-linewise and -wordwise,
so it's not unlikely that there could be other applications.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Eli Zaretskii]

> Date: Sun, 25 Mar 2018 22:29:29 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> CC: kfogel@red-bean.com, emacs-devel@gnu.org
> 
> > > I searched for "override functions" and "override primitives" in the
> > > index, but did not find a result.
> > 
> > I would index this as "redefining functions", at least in addition to
> > "overriding", if not instead of it.
> 
> I think both are good, although an index entry for "overriding
> functions" could also point to the "advice" documentation, since
> "overriding" is a less precise term than "redefining".

Done.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Juri Linkov]

>> > Instead of defining transpose-regions (and its interactive spec) in
>> > Lisp (as the reverted patch did), we could take the current
>> > interactive spec of transpose-regions and use that to make a preloaded
>> > function named "read-two-regions" in simple.el.  That would allow its
>> > reuse by other commands, too.
>>
>> Where would such a function be useful?
>
> Nothing specific comes to mind.  There is one pair of commands that
> already reads multiple regions: ediff-regions-linewise and -wordwise,
> so it's not unlikely that there could be other applications.

Why not use a compromise and move only the interactive spec to simple.el:

(put 'transpose-regions
     'interactive-form
     '(if (< (length mark-ring) 2)
	  (error "Other region must be marked before transposing two regions")
	(let* ((num (if current-prefix-arg
			(prefix-numeric-value current-prefix-arg)
		      0))
	       (ring-length (length mark-ring))
	       (eltnum (mod num ring-length))
	       (eltnum2 (mod (1+ num) ring-length)))
	  (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2)))))

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Karl Fogel]

Juri Linkov <juri@linkov.net> writes:
>Why not use a compromise and move only the interactive spec to simple.el:
>
>(put 'transpose-regions
>     'interactive-form
>     '(if (< (length mark-ring) 2)
>	  (error "Other region must be marked before transposing two regions")
>	(let* ((num (if current-prefix-arg
>			(prefix-numeric-value current-prefix-arg)
>		      0))
>	       (ring-length (length mark-ring))
>	       (eltnum (mod num ring-length))
>	       (eltnum2 (mod (1+ num) ring-length)))
>	  (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2)))))

Personally, I think I'd prefer to have the interactive form together with the rest of the function definition, even if that's in C.

However, the method you suggest is a nice way of overriding the interactive spec of a C-defined Lisp function, for example for when one wants to experiment with ways to improve the interactive behavior.

What would you think of mentioning this trick in doc/lispref/internals.texi, in the text for "@item interactive"?

Best regards,
-Karl

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Wed, 28 Mar 2018 23:53:53 -0500
> Cc: emacs-devel@gnu.org
> 
> >(put 'transpose-regions
> >     'interactive-form
> >     '(if (< (length mark-ring) 2)
> >	  (error "Other region must be marked before transposing two regions")
> >	(let* ((num (if current-prefix-arg
> >			(prefix-numeric-value current-prefix-arg)
> >		      0))
> >	       (ring-length (length mark-ring))
> >	       (eltnum (mod num ring-length))
> >	       (eltnum2 (mod (1+ num) ring-length)))
> >	  (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2)))))
> 
> Personally, I think I'd prefer to have the interactive form together with the rest of the function definition, even if that's in C.

I agree.  My reason is that this separation makes finding all the
pieces of the puzzle significantly harder.

> What would you think of mentioning this trick in doc/lispref/internals.texi, in the text for "@item interactive"?

The property is already prominently described, so just a
cross-reference to one of those places should be enough.

Re: [Emacs-diffs] master b88e7c8: Make transpose-regions interactive (Bug#30343)

[Stefan Monnier]

> Personally, I think I'd prefer to have the interactive form together with
> the rest of the function definition, even if that's in C.

Agreed.  I'd be fine separating them if the interactive spec is made
"generic" (as in the previously suggested definition of
"read-2-regions"), but with an open-coded spec, I'd rather have it right
next to the function.


        Stefan