bug#45550: [PATCH] Factor out new function for readability in chartab.c

bug#45550: [PATCH] Factor out new function for readability in chartab.c

[Stefan Kangas]

[-- 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

bug#45550: [PATCH] Factor out new function for readability in chartab.c

[Eli Zaretskii]

> 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.

bug#45550: [PATCH] Factor out new function for readability in chartab.c

[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 see that
it is used quite a lot in dispextern.c.  Otherwise, I can of course
rewrite it to use a macro instead.

bug#45550: [PATCH] Factor out new function for readability in chartab.c

[Eli Zaretskii]

> 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.

bug#45550: [PATCH] Factor out new function for readability in chartab.c

[Lars Ingebrigtsen]

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