* bug#44471: [PATCH] Simplify text-quoting-style
@ 2020-11-05 16:18 Stefan Kangas
2020-11-09 15:56 ` Lars Ingebrigtsen
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Kangas @ 2020-11-05 16:18 UTC (permalink / raw)
To: 44471
[-- 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* bug#44471: [PATCH] Simplify text-quoting-style
2020-11-05 16:18 bug#44471: [PATCH] Simplify text-quoting-style Stefan Kangas
@ 2020-11-09 15:56 ` Lars Ingebrigtsen
2020-11-09 20:13 ` Stefan Kangas
0 siblings, 1 reply; 7+ messages in thread
From: Lars Ingebrigtsen @ 2020-11-09 15:56 UTC (permalink / raw)
To: Stefan Kangas; +Cc: 44471
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#44471: [PATCH] Simplify text-quoting-style
2020-11-09 15:56 ` Lars Ingebrigtsen
@ 2020-11-09 20:13 ` Stefan Kangas
2020-11-10 14:30 ` Lars Ingebrigtsen
2020-11-10 16:12 ` Eli Zaretskii
0 siblings, 2 replies; 7+ messages in thread
From: Stefan Kangas @ 2020-11-09 20:13 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 44471
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).
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#44471: [PATCH] Simplify text-quoting-style
2020-11-09 20:13 ` Stefan Kangas
@ 2020-11-10 14:30 ` Lars Ingebrigtsen
2020-11-10 16:12 ` Eli Zaretskii
1 sibling, 0 replies; 7+ messages in thread
From: Lars Ingebrigtsen @ 2020-11-10 14:30 UTC (permalink / raw)
To: Stefan Kangas; +Cc: 44471
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#44471: [PATCH] Simplify text-quoting-style
2020-11-09 20:13 ` Stefan Kangas
2020-11-10 14:30 ` Lars Ingebrigtsen
@ 2020-11-10 16:12 ` Eli Zaretskii
2020-11-11 19:12 ` Stefan Kangas
1 sibling, 1 reply; 7+ messages in thread
From: Eli Zaretskii @ 2020-11-10 16:12 UTC (permalink / raw)
To: Stefan Kangas; +Cc: larsi, 44471
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#44471: [PATCH] Simplify text-quoting-style
2020-11-10 16:12 ` Eli Zaretskii
@ 2020-11-11 19:12 ` Stefan Kangas
2020-11-11 19:24 ` Eli Zaretskii
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Kangas @ 2020-11-11 19:12 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: larsi, 44471
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#44471: [PATCH] Simplify text-quoting-style
2020-11-11 19:12 ` Stefan Kangas
@ 2020-11-11 19:24 ` Eli Zaretskii
0 siblings, 0 replies; 7+ messages in thread
From: Eli Zaretskii @ 2020-11-11 19:24 UTC (permalink / raw)
To: Stefan Kangas; +Cc: larsi, 44471
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-11 19:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-05 16:18 bug#44471: [PATCH] Simplify text-quoting-style Stefan Kangas
2020-11-09 15:56 ` Lars Ingebrigtsen
2020-11-09 20:13 ` Stefan Kangas
2020-11-10 14:30 ` Lars Ingebrigtsen
2020-11-10 16:12 ` Eli Zaretskii
2020-11-11 19:12 ` Stefan Kangas
2020-11-11 19:24 ` Eli Zaretskii
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.