bug#44471: [PATCH] Simplify text-quoting-style

bug#44471: [PATCH] Simplify text-quoting-style

[Stefan Kangas]

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

I found an opportunity to simplify the code for text-quoting-style.  See
the attached patch.

The patch also improves the name of the defun `get-quoting-style' by
changing it to `text-quoting-style'.  (This new name matches the old C
function name and the existing variable name.)

Any comments?

[-- Attachment #2: 0001-Simplify-text-quoting-style.patch --]
[-- Type: text/x-diff, Size: 6474 bytes --]

From 6631890cc74c0c79a2ae20fe9f0b1bd66706b4c9 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 5 Nov 2020 15:32:45 +0100
Subject: [PATCH] Simplify text-quoting-style

* src/doc.c (text_quoting_style): Remove function by merging it...
(Ftext_quoting_style): ...here.  Rename from Fget_quoting_style.
(syms_of_doc): Update defsubr for Ftext_quoting_style.
* src/lisp.h (enum text_quoting_style): Remove enum.
* src/doprnt.c (doprnt):
* src/editfns.c (styled_format):
* lisp/help.el (substitute-command-keys): Update callers to use
text-quoting-style.
---
 lisp/help.el  |  4 ++--
 src/doc.c     | 40 ++++++++++++++--------------------------
 src/doprnt.c  |  8 ++++----
 src/editfns.c |  6 +++---
 src/lisp.h    | 12 ------------
 5 files changed, 23 insertions(+), 47 deletions(-)

diff --git a/lisp/help.el b/lisp/help.el
index 466ff21eb2..32ee84b5f9 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1103,13 +1103,13 @@ substitute-command-keys
                       (describe-map-tree this-keymap t (nreverse earlier-maps)
                                          nil nil t nil nil t))))))))
              ;; 2. Handle quotes.
-             ((and (eq (get-quoting-style) 'curve)
+             ((and (eq (text-quoting-style) 'curve)
                    (or (and (= (following-char) ?\`)
                             (prog1 t (insert "‘")))
                        (and (= (following-char) ?')
                             (prog1 t (insert "’")))))
               (delete-char 1))
-             ((and (eq (get-quoting-style) 'straight)
+             ((and (eq (text-quoting-style) 'straight)
                    (= (following-char) ?\`))
               (insert "'")
               (delete-char 1))
diff --git a/src/doc.c b/src/doc.c
index f1ce266d39..5f23e3d0bb 100644
--- a/src/doc.c
+++ b/src/doc.c
@@ -682,37 +682,25 @@ default_to_grave_quoting_style (void)
 	  && EQ (AREF (dv, 0), make_fixnum ('`')));
 }
 
-/* Return the current effective text quoting style.  */
-enum text_quoting_style
-text_quoting_style (void)
+DEFUN ("text-quoting-style", Ftext_quoting_style,
+       Stext_quoting_style, 0, 0, 0,
+       doc: /* Return the current effective text quoting style.
+See variable `text-quoting-style'.  */)
+  (void)
 {
+  /* Use grave accent and apostrophe `like this'.  */
   if (NILP (Vtext_quoting_style)
       ? default_to_grave_quoting_style ()
       : EQ (Vtext_quoting_style, Qgrave))
-    return GRAVE_QUOTING_STYLE;
+    return Qgrave;
+
+  /* Use apostrophes 'like this'.  */
   else if (EQ (Vtext_quoting_style, Qstraight))
-    return STRAIGHT_QUOTING_STYLE;
-  else
-    return CURVE_QUOTING_STYLE;
-}
+    return Qstraight;
 
-/* This is just a Lisp wrapper for text_quoting_style above.  */
-DEFUN ("get-quoting-style", Fget_quoting_style,
-       Sget_quoting_style, 0, 0, 0,
-       doc: /* Return the current effective text quoting style.
-See variable `text-quoting-style'.  */)
-  (void)
-{
-  switch (text_quoting_style ())
-    {
-    case STRAIGHT_QUOTING_STYLE:
-      return Qstraight;
-    case CURVE_QUOTING_STYLE:
-      return Qcurve;
-    case GRAVE_QUOTING_STYLE:
-    default:
-      return Qgrave;
-    }
+  /* Use curved single quotes ‘like this’.  */
+  else
+    return Qcurve;
 }
 
 \f
@@ -755,5 +743,5 @@ syms_of_doc (void)
   defsubr (&Sdocumentation);
   defsubr (&Sdocumentation_property);
   defsubr (&Ssnarf_documentation);
-  defsubr (&Sget_quoting_style);
+  defsubr (&Stext_quoting_style);
 }
diff --git a/src/doprnt.c b/src/doprnt.c
index ce259d07cf..9316497720 100644
--- a/src/doprnt.c
+++ b/src/doprnt.c
@@ -199,7 +199,7 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
   /* Buffer we have got with malloc.  */
   char *big_buffer = NULL;
 
-  enum text_quoting_style quoting_style = text_quoting_style ();
+  Lisp_Object quoting_style = Ftext_quoting_style ();
 
   bufsize--;
 
@@ -482,13 +482,13 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
 
       char const *src;
       ptrdiff_t srclen;
-      if (quoting_style == CURVE_QUOTING_STYLE && fmtchar == '`')
+      if (EQ (quoting_style, Qcurve) && fmtchar == '`')
 	src = uLSQM, srclen = sizeof uLSQM - 1;
-      else if (quoting_style == CURVE_QUOTING_STYLE && fmtchar == '\'')
+      else if (EQ (quoting_style, Qcurve) && fmtchar == '\'')
 	src = uRSQM, srclen = sizeof uRSQM - 1;
       else
 	{
-	  if (quoting_style == STRAIGHT_QUOTING_STYLE && fmtchar == '`')
+	  if (EQ (quoting_style, Qstraight) && fmtchar == '`')
 	    fmtchar = '\'';
 	  eassert (ASCII_CHAR_P (fmtchar));
 	  *bufptr++ = fmtchar;
diff --git a/src/editfns.c b/src/editfns.c
index ca6b8981eb..320fbdc669 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3147,7 +3147,7 @@ styled_format (ptrdiff_t nargs, Lisp_Object *args, bool message)
     if (STRINGP (args[i]) && STRING_MULTIBYTE (args[i]))
       multibyte = true;
 
-  int quoting_style = message ? text_quoting_style () : -1;
+  Lisp_Object quoting_style = message ? Ftext_quoting_style () : Qnil;
 
   ptrdiff_t ispec;
   ptrdiff_t nspec = 0;
@@ -3767,7 +3767,7 @@ styled_format (ptrdiff_t nargs, Lisp_Object *args, bool message)
 	  unsigned char str[MAX_MULTIBYTE_LENGTH];
 
 	  if ((format_char == '`' || format_char == '\'')
-	      && quoting_style == CURVE_QUOTING_STYLE)
+	      && EQ (quoting_style, Qcurve))
 	    {
 	      if (! multibyte)
 		{
@@ -3778,7 +3778,7 @@ styled_format (ptrdiff_t nargs, Lisp_Object *args, bool message)
 	      convbytes = 3;
 	      new_result = true;
 	    }
-	  else if (format_char == '`' && quoting_style == STRAIGHT_QUOTING_STYLE)
+	  else if (format_char == '`' && EQ (quoting_style, Qstraight))
 	    {
 	      convsrc = "'";
 	      new_result = true;
diff --git a/src/lisp.h b/src/lisp.h
index a3cfb5044d..c6eb44d74e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4495,18 +4495,6 @@ #define DAEMON_RUNNING (w32_daemon_event != INVALID_HANDLE_VALUE)
 extern void syms_of_callproc (void);
 
 /* Defined in doc.c.  */
-enum text_quoting_style
-  {
-    /* Use curved single quotes ‘like this’.  */
-    CURVE_QUOTING_STYLE,
-
-    /* Use grave accent and apostrophe  `like this'.  */
-    GRAVE_QUOTING_STYLE,
-
-    /* Use apostrophes 'like this'.  */
-    STRAIGHT_QUOTING_STYLE
-  };
-extern enum text_quoting_style text_quoting_style (void);
 extern Lisp_Object read_doc_string (Lisp_Object);
 extern Lisp_Object get_doc_string (Lisp_Object, bool, bool);
 extern void syms_of_doc (void);
-- 
2.28.0

bug#44471: [PATCH] Simplify text-quoting-style

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I found an opportunity to simplify the code for text-quoting-style.  See
> the attached patch.
>
> The patch also improves the name of the defun `get-quoting-style' by
> changing it to `text-quoting-style'.  (This new name matches the old C
> function name and the existing variable name.)
>
> Any comments?

Makes sense to me.  And since get-quoting-style is new in Emacs 28 (I
think?), no deprecation should be necessary.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#44471: [PATCH] Simplify text-quoting-style

[Stefan Kangas]

tags 44471 fixed
close 44471 28.1
thanks

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> I found an opportunity to simplify the code for text-quoting-style.  See
>> the attached patch.
>>
>> The patch also improves the name of the defun `get-quoting-style' by
>> changing it to `text-quoting-style'.  (This new name matches the old C
>> function name and the existing variable name.)
>>
>> Any comments?
>
> Makes sense to me.

Thanks.  There have been no other comments, so I've pushed this to
master as commit 95c04675ab.

> And since get-quoting-style is new in Emacs 28 (I think?), no
> deprecation should be necessary.

Yes, it is not even a month old (and added by yours truly).

bug#44471: [PATCH] Simplify text-quoting-style

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Yes, it is not even a month old (and added by yours truly).

Right.  :-)  Perhaps this function should have a NEWS entry?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#44471: [PATCH] Simplify text-quoting-style

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 9 Nov 2020 12:13:23 -0800
> Cc: 44471@debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Stefan Kangas <stefan@marxist.se> writes:
> >
> >> I found an opportunity to simplify the code for text-quoting-style.  See
> >> the attached patch.
> >>
> >> The patch also improves the name of the defun `get-quoting-style' by
> >> changing it to `text-quoting-style'.  (This new name matches the old C
> >> function name and the existing variable name.)
> >>
> >> Any comments?
> >
> > Makes sense to me.
> 
> Thanks.  There have been no other comments, so I've pushed this to
> master as commit 95c04675ab.

Please in the future allow more than just a couple of days for people
to comment on non-trivial patches.

As it happens, I do have a few issues with the change:

  . it slows down code of two very popular functions, because we now
    use EQ instead of a C-level equality operator;
  . it introduces a function that has the same name as a variable,
    which adds a bit to confusion, and also defeats our method of
    reporting in what version was the function/variable introduced
    (try "C-h f"); and
  . I don't think I see the simplification that justifies these
    (admittedly quite minor) downsides

Thanks.

bug#44471: [PATCH] Simplify text-quoting-style

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Please in the future allow more than just a couple of days for people
> to comment on non-trivial patches.

OK.

> As it happens, I do have a few issues with the change:
>
>   . it slows down code of two very popular functions, because we now
>     use EQ instead of a C-level equality operator;

If I understand correctly, EQ(x, y) ends up in a C-level equality
operator and a typecast.  Is that correct?  If yes, is it the typecast
that you think will cause a slowdown, or is it something else?

I naively thought that this would not make much of a difference, and
that, to the extent that it did, the relevant functions are hardly
called often enough to matter in the end.  But I assume you know more
than me here, so please tell me where I'm wrong.

>   . it introduces a function that has the same name as a variable,
>     which adds a bit to confusion, and also defeats our method of
>     reporting in what version was the function/variable introduced
>     (try "C-h f"); and

This is not without precedent, see e.g. `user-full-name'.  But if we
want to keep my patch we could of course just use the previous name
`get-text-style' instead.

>   . I don't think I see the simplification that justifies these
>     (admittedly quite minor) downsides

I agree it is minor, but I see one function less, one less enum, and
overall fewer lines of code.

I'm happy to revert it if the change is not wanted.

bug#44471: [PATCH] Simplify text-quoting-style

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 11 Nov 2020 14:12:24 -0500
> Cc: larsi@gnus.org, 44471@debbugs.gnu.org
> 
> >   . it slows down code of two very popular functions, because we now
> >     use EQ instead of a C-level equality operator;
> 
> If I understand correctly, EQ(x, y) ends up in a C-level equality
> operator and a typecast.  Is that correct?

No.  EQ either expands to a call to lisp_h_EQ, or calls lisp_h_EQ,
which does:

  #define lisp_h_EQ(x, y) (XLI (x) == XLI (y))

Depending on how Lisp_Object is represented, this could be a typecast
or an access to a struct member.

> If yes, is it the typecast
> that you think will cause a slowdown, or is it something else?

the function call and the struct access.

> I naively thought that this would not make much of a difference, and
> that, to the extent that it did, the relevant functions are hardly
> called often enough to matter in the end.

The difference is very minor, but the point is that it's a (minor)
disadvantage.

> >   . it introduces a function that has the same name as a variable,
> >     which adds a bit to confusion, and also defeats our method of
> >     reporting in what version was the function/variable introduced
> >     (try "C-h f"); and
> 
> This is not without precedent, see e.g. `user-full-name'.  But if we
> want to keep my patch we could of course just use the previous name
> `get-text-style' instead.
> 
> >   . I don't think I see the simplification that justifies these
> >     (admittedly quite minor) downsides
> 
> I agree it is minor, but I see one function less, one less enum, and
> overall fewer lines of code.
> 
> I'm happy to revert it if the change is not wanted.

I don't have strong opinions, so I will leave it to you and others.

My point, though, is that minor cleanups should preferably not have
adverse effects, otherwise their benefit is questionable.