[PATCH] ash, lsh: Avoid code duplication

[PATCH] ash, lsh: Avoid code duplication

[Tino Calancha]

Hi,

`ash' and `lsh' show some overlap.
How about following patch?

Thank you.
Tino


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From ad93a75357efefa850e6030a2a34aa946fd65332 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Mon, 21 Nov 2016 22:44:53 +0900
Subject: [PATCH] ash, lsh avoid code duplication

* src/data.c (ash_lsh_impl): New function.
(ash, lsh): Use it.
* src/lisp.h (ash_lsh_impl): Declare it.
---
 src/data.c | 36 +++++++++++++++---------------------
 src/lisp.h |  1 +
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/src/data.c b/src/data.c
index d221db4..9391943 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2924,11 +2924,8 @@ usage: (logxor &rest INTS-OR-MARKERS)  */)
   return arith_driver (Alogxor, nargs, args);
 }
 
-DEFUN ("ash", Fash, Sash, 2, 2, 0,
-       doc: /* Return VALUE with its bits shifted left by COUNT.
-If COUNT is negative, shifting is actually to the right.
-In this case, the sign bit is duplicated.  */)
-  (register Lisp_Object value, Lisp_Object count)
+Lisp_Object
+ash_lsh_impl (register Lisp_Object value, Lisp_Object count, int lsh)
 {
   register Lisp_Object val;
 
@@ -2940,32 +2937,29 @@ In this case, the sign bit is duplicated.  */)
   else if (XINT (count) > 0)
     XSETINT (val, XUINT (value) << XFASTINT (count));
   else if (XINT (count) <= -EMACS_INT_WIDTH)
-    XSETINT (val, XINT (value) < 0 ? -1 : 0);
+    XSETINT (val, lsh == 1 ? 0 : XINT (value) < 0 ? -1 : 0);
   else
-    XSETINT (val, XINT (value) >> -XINT (count));
+    XSETINT (val, lsh == 1 ? XUINT (value) >> -XINT (count) : \
+                             XINT (value) >> -XINT (count));
   return val;
 }
 
+DEFUN ("ash", Fash, Sash, 2, 2, 0,
+       doc: /* Return VALUE with its bits shifted left by COUNT.
+If COUNT is negative, shifting is actually to the right.
+In this case, the sign bit is duplicated.  */)
+  (register Lisp_Object value, Lisp_Object count)
+{
+  return ash_lsh_impl (value, count, 0);
+}
+
 DEFUN ("lsh", Flsh, Slsh, 2, 2, 0,
        doc: /* Return VALUE with its bits shifted left by COUNT.
 If COUNT is negative, shifting is actually to the right.
 In this case, zeros are shifted in on the left.  */)
   (register Lisp_Object value, Lisp_Object count)
 {
-  register Lisp_Object val;
-
-  CHECK_NUMBER (value);
-  CHECK_NUMBER (count);
-
-  if (XINT (count) >= EMACS_INT_WIDTH)
-    XSETINT (val, 0);
-  else if (XINT (count) > 0)
-    XSETINT (val, XUINT (value) << XFASTINT (count));
-  else if (XINT (count) <= -EMACS_INT_WIDTH)
-    XSETINT (val, 0);
-  else
-    XSETINT (val, XUINT (value) >> -XINT (count));
-  return val;
+  return ash_lsh_impl (value, count, 1);
 }
 
 DEFUN ("1+", Fadd1, Sadd1, 1, 1, 0,
diff --git a/src/lisp.h b/src/lisp.h
index e087828..00687d7 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
 /* Defined in data.c.  */
 extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
 extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
+extern Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, int);
 
 /* Defined in emacs.c.  */
 #ifdef DOUG_LEA_MALLOC
-- 
2.10.2

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.3)
 of 2016-11-21
Repository revision: 209d85fe5691a9334e14bc63b0c836880831a054

Re: [PATCH] ash, lsh: Avoid code duplication

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Mon, 21 Nov 2016 23:45:36 +0900
> Cc: tino.calancha@gmail.com
> 
> `ash' and `lsh' show some overlap.
> How about following patch?

With some more work (see below), and if no one objects, why not?

> +Lisp_Object
> +ash_lsh_impl (register Lisp_Object value, Lisp_Object count, int lsh)
                                                                ^^^^^^^
'bool', please.

Also, the function doesn't need to be extern, it should be static.

> -    XSETINT (val, XINT (value) < 0 ? -1 : 0);
> +    XSETINT (val, lsh == 1 ? 0 : XINT (value) < 0 ? -1 : 0);
                     ^^^^^^^^
a.k.a. "lsh ?"

> +DEFUN ("ash", Fash, Sash, 2, 2, 0,
> +       doc: /* Return VALUE with its bits shifted left by COUNT.
> +If COUNT is negative, shifting is actually to the right.
> +In this case, the sign bit is duplicated.  */)
> +  (register Lisp_Object value, Lisp_Object count)
> +{
> +  return ash_lsh_impl (value, count, 0);
                                        ^
'false', please.

> +  return ash_lsh_impl (value, count, 1);
                                        ^
'true', please.

> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
>  /* Defined in data.c.  */
>  extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
>  extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
> +extern Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, int);

No need to make it extern, as no other C file uses it.

Re: [PATCH] ash, lsh: Avoid code duplication

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Mon, 21 Nov 2016 23:45:36 +0900
>> Cc: tino.calancha@gmail.com
>> 
>> `ash' and `lsh' show some overlap.
>> How about following patch?
>
> With some more work (see below), and if no one objects, why not?
>
>> +Lisp_Object
>> +ash_lsh_impl (register Lisp_Object value, Lisp_Object count, int lsh)
>                                                                 ^^^^^^^
> 'bool', please.
>
>> --- a/src/lisp.h
>> +++ b/src/lisp.h
>> @@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
>>  /* Defined in data.c.  */
>>  extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
>>  extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
>> +extern Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, int);
>
> No need to make it extern, as no other C file uses it.

Thank you.  Following is the new patch after your comments:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 8ca2e201efcbf93b0e7450cc1a72bb3f6c6f23d4 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Tue, 22 Nov 2016 01:09:50 +0900
Subject: [PATCH] ash, lsh avoid code duplication

* src/data.c (ash_lsh_impl): New function.
(ash, lsh): Use it.
* src/lisp.h (ash_lsh_impl): Declare it.
---
 src/data.c | 36 +++++++++++++++---------------------
 src/lisp.h |  1 +
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/src/data.c b/src/data.c
index d221db4..a34230f 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2924,11 +2924,8 @@ usage: (logxor &rest INTS-OR-MARKERS)  */)
   return arith_driver (Alogxor, nargs, args);
 }
 
-DEFUN ("ash", Fash, Sash, 2, 2, 0,
-       doc: /* Return VALUE with its bits shifted left by COUNT.
-If COUNT is negative, shifting is actually to the right.
-In this case, the sign bit is duplicated.  */)
-  (register Lisp_Object value, Lisp_Object count)
+Lisp_Object
+ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)
 {
   register Lisp_Object val;
 
@@ -2940,32 +2937,29 @@ In this case, the sign bit is duplicated.  */)
   else if (XINT (count) > 0)
     XSETINT (val, XUINT (value) << XFASTINT (count));
   else if (XINT (count) <= -EMACS_INT_WIDTH)
-    XSETINT (val, XINT (value) < 0 ? -1 : 0);
+    XSETINT (val, lsh ? 0 : XINT (value) < 0 ? -1 : 0);
   else
-    XSETINT (val, XINT (value) >> -XINT (count));
+    XSETINT (val, lsh ? XUINT (value) >> -XINT (count) : \
+                        XINT (value) >> -XINT (count));
   return val;
 }
 
+DEFUN ("ash", Fash, Sash, 2, 2, 0,
+       doc: /* Return VALUE with its bits shifted left by COUNT.
+If COUNT is negative, shifting is actually to the right.
+In this case, the sign bit is duplicated.  */)
+  (register Lisp_Object value, Lisp_Object count)
+{
+  return ash_lsh_impl (value, count, false);
+}
+
 DEFUN ("lsh", Flsh, Slsh, 2, 2, 0,
        doc: /* Return VALUE with its bits shifted left by COUNT.
 If COUNT is negative, shifting is actually to the right.
 In this case, zeros are shifted in on the left.  */)
   (register Lisp_Object value, Lisp_Object count)
 {
-  register Lisp_Object val;
-
-  CHECK_NUMBER (value);
-  CHECK_NUMBER (count);
-
-  if (XINT (count) >= EMACS_INT_WIDTH)
-    XSETINT (val, 0);
-  else if (XINT (count) > 0)
-    XSETINT (val, XUINT (value) << XFASTINT (count));
-  else if (XINT (count) <= -EMACS_INT_WIDTH)
-    XSETINT (val, 0);
-  else
-    XSETINT (val, XUINT (value) >> -XINT (count));
-  return val;
+  return ash_lsh_impl (value, count, true);
 }
 
 DEFUN ("1+", Fadd1, Sadd1, 1, 1, 0,
diff --git a/src/lisp.h b/src/lisp.h
index e087828..c48c2c8 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
 /* Defined in data.c.  */
 extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
 extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
+Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, bool);
 
 /* Defined in emacs.c.  */
 #ifdef DOUG_LEA_MALLOC
-- 
2.10.2

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.3)
 of 2016-11-21
Repository revision: 209d85fe5691a9334e14bc63b0c836880831a054

Re: [PATCH] ash, lsh: Avoid code duplication

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> CC: emacs-devel@gnu.org, Tino Calancha <tino.calancha@gmail.com>
> Date: Tue, 22 Nov 2016 01:20:51 +0900
> 
> Thank you.  Following is the new patch after your comments:

We are close.

> +Lisp_Object
> +ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)

static Lisp_Object
ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)

> diff --git a/src/lisp.h b/src/lisp.h
> index e087828..c48c2c8 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
>  /* Defined in data.c.  */
>  extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
>  extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
> +Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, bool);

No need to declare it here, as it is a static function used only in
the file in which it is defined.

Thanks.

Re: [PATCH] ash, lsh: Avoid code duplication

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> +Lisp_Object
>> +ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)
>
> static Lisp_Object
> ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)
>
>> diff --git a/src/lisp.h b/src/lisp.h
>> index e087828..c48c2c8 100644
>> --- a/src/lisp.h
>> +++ b/src/lisp.h
>> @@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
>>  /* Defined in data.c.  */
>>  extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
>>  extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
>> +Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, bool);
>
> No need to declare it here, as it is a static function used only in
> the file in which it is defined.

Thank you very much.
Here is the updated patch:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 5b21b3ab846203c329710685433f16a78f1699f3 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Tue, 22 Nov 2016 11:12:51 +0900
Subject: [PATCH] ash, lsh avoid code duplication

* src/data.c (ash_lsh_impl): New function.
(ash, lsh): Use it.
---
 src/data.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/src/data.c b/src/data.c
index d221db4..61b5da8 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2924,11 +2924,8 @@ usage: (logxor &rest INTS-OR-MARKERS)  */)
   return arith_driver (Alogxor, nargs, args);
 }
 
-DEFUN ("ash", Fash, Sash, 2, 2, 0,
-       doc: /* Return VALUE with its bits shifted left by COUNT.
-If COUNT is negative, shifting is actually to the right.
-In this case, the sign bit is duplicated.  */)
-  (register Lisp_Object value, Lisp_Object count)
+static Lisp_Object
+ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)
 {
   register Lisp_Object val;
 
@@ -2940,32 +2937,29 @@ In this case, the sign bit is duplicated.  */)
   else if (XINT (count) > 0)
     XSETINT (val, XUINT (value) << XFASTINT (count));
   else if (XINT (count) <= -EMACS_INT_WIDTH)
-    XSETINT (val, XINT (value) < 0 ? -1 : 0);
+    XSETINT (val, lsh ? 0 : XINT (value) < 0 ? -1 : 0);
   else
-    XSETINT (val, XINT (value) >> -XINT (count));
+    XSETINT (val, lsh ? XUINT (value) >> -XINT (count) : \
+                        XINT (value) >> -XINT (count));
   return val;
 }
 
+DEFUN ("ash", Fash, Sash, 2, 2, 0,
+       doc: /* Return VALUE with its bits shifted left by COUNT.
+If COUNT is negative, shifting is actually to the right.
+In this case, the sign bit is duplicated.  */)
+  (register Lisp_Object value, Lisp_Object count)
+{
+  return ash_lsh_impl (value, count, false);
+}
+
 DEFUN ("lsh", Flsh, Slsh, 2, 2, 0,
        doc: /* Return VALUE with its bits shifted left by COUNT.
 If COUNT is negative, shifting is actually to the right.
 In this case, zeros are shifted in on the left.  */)
   (register Lisp_Object value, Lisp_Object count)
 {
-  register Lisp_Object val;
-
-  CHECK_NUMBER (value);
-  CHECK_NUMBER (count);
-
-  if (XINT (count) >= EMACS_INT_WIDTH)
-    XSETINT (val, 0);
-  else if (XINT (count) > 0)
-    XSETINT (val, XUINT (value) << XFASTINT (count));
-  else if (XINT (count) <= -EMACS_INT_WIDTH)
-    XSETINT (val, 0);
-  else
-    XSETINT (val, XUINT (value) >> -XINT (count));
-  return val;
+  return ash_lsh_impl (value, count, true);
 }
 
 DEFUN ("1+", Fadd1, Sadd1, 1, 1, 0,
-- 
2.10.2

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.3)
 of 2016-11-22 built on calancha-pc
Repository revision: f9946cab7e30a7e01806c4d6273a9251a4504c16

Re: [PATCH] ash, lsh: Avoid code duplication

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> CC: emacs-devel@gnu.org, Tino Calancha <tino.calancha@gmail.com>
> Date: Tue, 22 Nov 2016 11:17:24 +0900
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> +Lisp_Object
> >> +ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)
> >
> > static Lisp_Object
> > ash_lsh_impl (register Lisp_Object value, Lisp_Object count, bool lsh)
> >
> >> diff --git a/src/lisp.h b/src/lisp.h
> >> index e087828..c48c2c8 100644
> >> --- a/src/lisp.h
> >> +++ b/src/lisp.h
> >> @@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
> >>  /* Defined in data.c.  */
> >>  extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
> >>  extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
> >> +Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, bool);
> >
> > No need to declare it here, as it is a static function used only in
> > the file in which it is defined.
> 
> Thank you very much.
> Here is the updated patch:

Thanks, this LGTM.  Please push to master if no further comments are
posted.

Re: [PATCH] ash, lsh: Avoid code duplication

[Tino Calancha]

On Tue, 22 Nov 2016, Eli Zaretskii wrote:

> Thanks, this LGTM.  Please push to master if no further comments are
> posted.
Thank you for the notice.  OK, I will wait a few days for comments.

Re: [PATCH] ash, lsh: Avoid code duplication

[Davis Herring]

>> --- a/src/lisp.h
>> +++ b/src/lisp.h
>> @@ -602,6 +602,7 @@ extern void char_table_set (Lisp_Object, int, Lisp_Object);
>>  /* Defined in data.c.  */
>>  extern _Noreturn Lisp_Object wrong_type_argument (Lisp_Object, Lisp_Object);
>>  extern _Noreturn void wrong_choice (Lisp_Object, Lisp_Object);
>> +Lisp_Object ash_lsh_impl (Lisp_Object, Lisp_Object, bool);
>
> No need to declare it here, as it is a static function used only in
> the file in which it is defined.

In case it helps explain the issue, note that the "extern" here did 
nothing substantive: functions are extern if not static, and the extern 
keyword isn't needed to prevent a function declaration being a definition.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or 
too sparse, it is because mass-energy conversion has occurred during 
shipping.

Re: [PATCH] ash, lsh: Avoid code duplication

[Tino Calancha]

Eli Zaretskii <eliz@gnu.org> writes:

>> Here is the updated patch:
>
> Thanks, this LGTM.  Please push to master if no further comments are
> posted.
Pushed to master branch as commit 416adda.