all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#45550: [PATCH] Factor out new function for readability in chartab.c
@ 2020-12-30  8:56 Stefan Kangas
  2020-12-30 20:15 ` Eli Zaretskii
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Kangas @ 2020-12-30  8:56 UTC (permalink / raw)
  To: 45550

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

Severity: wishlist

While poking around, I found an opportunity for refactoring in
charctab.c.  I find it makes these functions significantly easier to
read and understand.

Please see the attached patch.

[-- Attachment #2: 0001-Factor-out-new-function-for-readability.patch --]
[-- Type: text/x-diff, Size: 6367 bytes --]

From 2365011892e4587cfdd3c3f0d0501c4afef2c441 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 30 Dec 2020 10:14:20 +0100
Subject: [PATCH] Factor out new function for readability

* src/chartab.c (sub_char_table_ref_and_range): New function...
(sub_char_table_ref_and_range, char_table_ref_and_range):
...factored out from here.
---
 src/chartab.c | 104 +++++++++++++++++++++-----------------------------
 1 file changed, 44 insertions(+), 60 deletions(-)

diff --git a/src/chartab.c b/src/chartab.c
index 331e8595eb..933853ed18 100644
--- a/src/chartab.c
+++ b/src/chartab.c
@@ -62,6 +62,9 @@ #define CHARTAB_IDX(c, depth, min_char)		\
 
 static Lisp_Object uniprop_table_uncompress (Lisp_Object, int);
 static uniprop_decoder_t uniprop_get_decoder (Lisp_Object);
+static Lisp_Object
+sub_char_table_ref_and_range (Lisp_Object, int, int *, int *,
+			      Lisp_Object, bool);
 
 /* 1 iff TABLE is a uniprop table.  */
 #define UNIPROP_TABLE_P(TABLE)					\
@@ -247,6 +250,23 @@ char_table_ref (Lisp_Object table, int c)
   return val;
 }
 
+static Lisp_Object
+char_table_ref_simple (Lisp_Object table, int idx, int c, int *from, int *to,
+		       Lisp_Object defalt, bool is_uniprop, bool is_subtable)
+{
+  Lisp_Object val = is_subtable ?
+    XSUB_CHAR_TABLE (table)->contents[idx]:
+    XCHAR_TABLE (table)->contents[idx];
+  if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (val))
+    val = uniprop_table_uncompress (table, idx);
+  if (SUB_CHAR_TABLE_P (val))
+    val = sub_char_table_ref_and_range (val, c, from, to,
+					defalt, is_uniprop);
+  else if (NILP (val))
+    val = defalt;
+  return val;
+}
+
 static Lisp_Object
 sub_char_table_ref_and_range (Lisp_Object table, int c, int *from, int *to,
 			      Lisp_Object defalt, bool is_uniprop)
@@ -254,31 +274,18 @@ sub_char_table_ref_and_range (Lisp_Object table, int c, int *from, int *to,
   struct Lisp_Sub_Char_Table *tbl = XSUB_CHAR_TABLE (table);
   int depth = tbl->depth, min_char = tbl->min_char;
   int chartab_idx = CHARTAB_IDX (c, depth, min_char), idx;
-  Lisp_Object val;
-
-  val = tbl->contents[chartab_idx];
-  if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (val))
-    val = uniprop_table_uncompress (table, chartab_idx);
-  if (SUB_CHAR_TABLE_P (val))
-    val = sub_char_table_ref_and_range (val, c, from, to, defalt, is_uniprop);
-  else if (NILP (val))
-    val = defalt;
+  Lisp_Object val
+    = char_table_ref_simple (table, chartab_idx, c, from, to,
+			     defalt, is_uniprop, true);
 
   idx = chartab_idx;
   while (idx > 0 && *from < min_char + idx * chartab_chars[depth])
     {
-      Lisp_Object this_val;
-
       c = min_char + idx * chartab_chars[depth] - 1;
       idx--;
-      this_val = tbl->contents[idx];
-      if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (this_val))
-	this_val = uniprop_table_uncompress (table, idx);
-      if (SUB_CHAR_TABLE_P (this_val))
-	this_val = sub_char_table_ref_and_range (this_val, c, from, to, defalt,
-						 is_uniprop);
-      else if (NILP (this_val))
-	this_val = defalt;
+      Lisp_Object this_val
+	= char_table_ref_simple (table, idx, c, from, to,
+				 defalt, is_uniprop, true);
 
       if (! EQ (this_val, val))
 	{
@@ -290,17 +297,11 @@ sub_char_table_ref_and_range (Lisp_Object table, int c, int *from, int *to,
 	  < chartab_chars[depth - 1])
 	 && (c += min_char) <= *to)
     {
-      Lisp_Object this_val;
-
       chartab_idx++;
-      this_val = tbl->contents[chartab_idx];
-      if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (this_val))
-	this_val = uniprop_table_uncompress (table, chartab_idx);
-      if (SUB_CHAR_TABLE_P (this_val))
-	this_val = sub_char_table_ref_and_range (this_val, c, from, to, defalt,
-						 is_uniprop);
-      else if (NILP (this_val))
-	this_val = defalt;
+      Lisp_Object this_val
+	= char_table_ref_simple (table, chartab_idx, c, from, to,
+				 defalt, is_uniprop, true);
+
       if (! EQ (this_val, val))
 	{
 	  *to = c - 1;
@@ -321,37 +322,26 @@ sub_char_table_ref_and_range (Lisp_Object table, int c, int *from, int *to,
 char_table_ref_and_range (Lisp_Object table, int c, int *from, int *to)
 {
   struct Lisp_Char_Table *tbl = XCHAR_TABLE (table);
-  int chartab_idx = CHARTAB_IDX (c, 0, 0), idx;
-  Lisp_Object val;
+  int chartab_idx = CHARTAB_IDX (c, 0, 0);
   bool is_uniprop = UNIPROP_TABLE_P (table);
 
-  val = tbl->contents[chartab_idx];
   if (*from < 0)
     *from = 0;
   if (*to < 0)
     *to = MAX_CHAR;
-  if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (val))
-    val = uniprop_table_uncompress (table, chartab_idx);
-  if (SUB_CHAR_TABLE_P (val))
-    val = sub_char_table_ref_and_range (val, c, from, to, tbl->defalt,
-					is_uniprop);
-  else if (NILP (val))
-    val = tbl->defalt;
-  idx = chartab_idx;
+
+  Lisp_Object val
+    = char_table_ref_simple (table, chartab_idx, c, from, to,
+			     tbl->defalt, is_uniprop, false);
+
+  int idx = chartab_idx;
   while (*from < idx * chartab_chars[0])
     {
-      Lisp_Object this_val;
-
       c = idx * chartab_chars[0] - 1;
       idx--;
-      this_val = tbl->contents[idx];
-      if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (this_val))
-	this_val = uniprop_table_uncompress (table, idx);
-      if (SUB_CHAR_TABLE_P (this_val))
-	this_val = sub_char_table_ref_and_range (this_val, c, from, to,
-						 tbl->defalt, is_uniprop);
-      else if (NILP (this_val))
-	this_val = tbl->defalt;
+      Lisp_Object this_val
+	= char_table_ref_simple (table, idx, c, from, to,
+				 tbl->defalt, is_uniprop, false);
 
       if (! EQ (this_val, val))
 	{
@@ -361,18 +351,12 @@ char_table_ref_and_range (Lisp_Object table, int c, int *from, int *to)
     }
   while (*to >= (chartab_idx + 1) * chartab_chars[0])
     {
-      Lisp_Object this_val;
-
       chartab_idx++;
       c = chartab_idx * chartab_chars[0];
-      this_val = tbl->contents[chartab_idx];
-      if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (this_val))
-	this_val = uniprop_table_uncompress (table, chartab_idx);
-      if (SUB_CHAR_TABLE_P (this_val))
-	this_val = sub_char_table_ref_and_range (this_val, c, from, to,
-						 tbl->defalt, is_uniprop);
-      else if (NILP (this_val))
-	this_val = tbl->defalt;
+      Lisp_Object this_val
+	= char_table_ref_simple (table, chartab_idx, c, from, to,
+				 tbl->defalt, is_uniprop, false);
+
       if (! EQ (this_val, val))
 	{
 	  *to = c - 1;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* bug#45550: [PATCH] Factor out new function for readability in chartab.c
  2020-12-30  8:56 bug#45550: [PATCH] Factor out new function for readability in chartab.c Stefan Kangas
@ 2020-12-30 20:15 ` Eli Zaretskii
  2020-12-30 21:16   ` Stefan Kangas
  0 siblings, 1 reply; 5+ messages in thread
From: Eli Zaretskii @ 2020-12-30 20:15 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 45550

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 30 Dec 2020 02:56:53 -0600
> 
> While poking around, I found an opportunity for refactoring in
> charctab.c.  I find it makes these functions significantly easier to
> read and understand.
> 
> Please see the attached patch.

Thanks, I have a couple of comments:

> +static Lisp_Object
> +sub_char_table_ref_and_range (Lisp_Object, int, int *, int *,
> +			      Lisp_Object, bool);

There should be no need to declare a static function which is defined
before it is used.

> +static Lisp_Object
> +char_table_ref_simple (Lisp_Object table, int idx, int c, int *from, int *to,
> +		       Lisp_Object defalt, bool is_uniprop, bool is_subtable)
> +{
> +  Lisp_Object val = is_subtable ?
> +    XSUB_CHAR_TABLE (table)->contents[idx]:
> +    XCHAR_TABLE (table)->contents[idx];
> +  if (is_uniprop && UNIPROP_COMPRESSED_FORM_P (val))
> +    val = uniprop_table_uncompress (table, idx);
> +  if (SUB_CHAR_TABLE_P (val))
> +    val = sub_char_table_ref_and_range (val, c, from, to,
> +					defalt, is_uniprop);
> +  else if (NILP (val))
> +    val = defalt;
> +  return val;
> +}
> +

Please make this a macro, not a function.  The code you are factoring
out is called in the innermost loops of the display engine, in bidi.c,
so it must be as fast as possible, even in an unoptimized build, where
static functions aren't inlined.





^ permalink raw reply	[flat|nested] 5+ messages in thread

* bug#45550: [PATCH] Factor out new function for readability in chartab.c
  2020-12-30 20:15 ` Eli Zaretskii
@ 2020-12-30 21:16   ` Stefan Kangas
  2020-12-31  3:29     ` Eli Zaretskii
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Kangas @ 2020-12-30 21:16 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 45550

Eli Zaretskii <eliz@gnu.org> writes:

> Please make this a macro, not a function.  The code you are factoring
> out is called in the innermost loops of the display engine, in bidi.c,
> so it must be as fast as possible, even in an unoptimized build, where
> static functions aren't inlined.

Would it be acceptable to use our INLINE macro here instead?  I see that
it is used quite a lot in dispextern.c.  Otherwise, I can of course
rewrite it to use a macro instead.





^ permalink raw reply	[flat|nested] 5+ messages in thread

* bug#45550: [PATCH] Factor out new function for readability in chartab.c
  2020-12-30 21:16   ` Stefan Kangas
@ 2020-12-31  3:29     ` Eli Zaretskii
  2021-07-21 12:03       ` Lars Ingebrigtsen
  0 siblings, 1 reply; 5+ messages in thread
From: Eli Zaretskii @ 2020-12-31  3:29 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 45550

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 30 Dec 2020 15:16:05 -0600
> Cc: 45550@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please make this a macro, not a function.  The code you are factoring
> > out is called in the innermost loops of the display engine, in bidi.c,
> > so it must be as fast as possible, even in an unoptimized build, where
> > static functions aren't inlined.
> 
> Would it be acceptable to use our INLINE macro here instead?

I'd prefer a simple macro.  I can never remember the exact semantics
of INLINE this week.

Thanks.





^ permalink raw reply	[flat|nested] 5+ messages in thread

* bug#45550: [PATCH] Factor out new function for readability in chartab.c
  2020-12-31  3:29     ` Eli Zaretskii
@ 2021-07-21 12:03       ` Lars Ingebrigtsen
  0 siblings, 0 replies; 5+ messages in thread
From: Lars Ingebrigtsen @ 2021-07-21 12:03 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 45550, Stefan Kangas

Eli Zaretskii <eliz@gnu.org> writes:

>> > Please make this a macro, not a function.  The code you are factoring
>> > out is called in the innermost loops of the display engine, in bidi.c,
>> > so it must be as fast as possible, even in an unoptimized build, where
>> > static functions aren't inlined.
>> 
>> Would it be acceptable to use our INLINE macro here instead?
>
> I'd prefer a simple macro.  I can never remember the exact semantics
> of INLINE this week.

I've applied Stefan's patch (with the "static inline" tweak, which we
use other places in Emacs, too) because patches bit-rot when they get
too old.  If it turns out to be a performance issue, then the inline
function can be rewritten as a macro.

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





^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-21 12:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-30  8:56 bug#45550: [PATCH] Factor out new function for readability in chartab.c Stefan Kangas
2020-12-30 20:15 ` Eli Zaretskii
2020-12-30 21:16   ` Stefan Kangas
2020-12-31  3:29     ` Eli Zaretskii
2021-07-21 12:03       ` Lars Ingebrigtsen

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.