bug#22630: [PATCH] Let assv/assoc shortcircuit to assq where feasible

bug#22630: [PATCH] Let assv/assoc shortcircuit to assq where feasible

[David Kastrup]

	* libguile/alist.c (scm_sloppy_assv, scm_sloppy_assoc):
	Shortcircuit to scm_sloppy_assq where feasible
	(scm_assv, scm_assoc): Shortcircuit to scm_assq where feasible
---
 libguile/alist.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/libguile/alist.c b/libguile/alist.c
index f33aa41..e9bb80e 100644
--- a/libguile/alist.c
+++ b/libguile/alist.c
@@ -28,6 +28,7 @@
 
 #include "libguile/validate.h"
 #include "libguile/pairs.h"
+#include "libguile/numbers.h"
 #include "libguile/alist.h"
 
 \f
@@ -72,6 +73,12 @@ SCM_DEFINE (scm_sloppy_assv, "sloppy-assv", 2, 0, 0,
 	    "Recommended only for use in Guile internals.")
 #define FUNC_NAME s_scm_sloppy_assv
 {
+  /* Non-immediate numbers are the only keys we need to check
+   * other than with eq
+   */
+  if (!SCM_NUMP (key))
+    return scm_sloppy_assq (key, alist);
+
   for (; scm_is_pair (alist); alist = SCM_CDR (alist))
     {
       SCM tmp = SCM_CAR (alist);
@@ -90,6 +97,10 @@ SCM_DEFINE (scm_sloppy_assoc, "sloppy-assoc", 2, 0, 0,
 	    "Recommended only for use in Guile internals.")
 #define FUNC_NAME s_scm_sloppy_assoc
 {
+  /* Immediate values can be checked using eq */
+  if (SCM_IMP (key))
+    return scm_sloppy_assq (key, alist);
+
   for (; scm_is_pair (alist); alist = SCM_CDR (alist))
     {
       SCM tmp = SCM_CAR (alist);
@@ -139,6 +150,13 @@ SCM_DEFINE (scm_assv, "assv", 2, 0, 0,
 #define FUNC_NAME s_scm_assv
 {
   SCM ls = alist;
+
+  /* Non-immediate numbers are the only keys we need to check
+   * other than with eq
+   */
+  if (!SCM_NUMP (key))
+    return scm_assq (key, alist);
+
   for(; scm_is_pair (ls); ls = SCM_CDR (ls)) 
     {
       SCM tmp = SCM_CAR (ls);
@@ -160,6 +178,11 @@ SCM_DEFINE (scm_assoc, "assoc", 2, 0, 0,
 #define FUNC_NAME s_scm_assoc
 {
   SCM ls = alist;
+
+  /* Immediate values can be checked using eq */
+  if (SCM_IMP (key))
+    return scm_assq (key, alist);
+
   for(; scm_is_pair (ls); ls = SCM_CDR (ls)) 
     {
       SCM tmp = SCM_CAR (ls);
-- 
2.5.0

bug#22630: [PATCH] Let assv/assoc shortcircuit to assq where feasible

[Andy Wingo]

On Thu 11 Feb 2016 12:31, David Kastrup <dak@gnu.org> writes:

> 	* libguile/alist.c (scm_sloppy_assv, scm_sloppy_assoc):
> 	Shortcircuit to scm_sloppy_assq where feasible
> 	(scm_assv, scm_assoc): Shortcircuit to scm_assq where feasible

Applied to master, will backport when I get a chance.  Thanks :)

Andy

bug#22630: Bad comment

[David Kastrup]

I routinely rebase before abandoning my patches so it only now caught my
attention that this bug fix was committed with changed comments.
Unfortunately, one of those comments was changed for the worse.

Here is the respective merge conflict:

<<<<<<< 4f324684cc7672b39800bce0a373da10057dc3c6
  /* In Guile, `assv' is the same as `assq' for keys of all types except
     numbers.  */
=======
  /* Non-immediate numbers are the only keys we need to check
   * other than with eq
   */
>>>>>>> Let assv/assoc shortcircuit to assq where feasible
  if (!SCM_NUMP (key))
    return scm_sloppy_assq (key, alist);

The first of the comments is the one actually in the repository.  Apart
from the difference in formatting (which is fine) it features a
different in content that is extremely misleading.  SCM_NUMP indeed
checks for _nonimmediate_ numbers (as opposed to the more inclusive
SCM_NUMBERP).  Since this comment concerns a _very_ deliberate and
nontrivial optimization and since the name of the macro SCM_NUMP is
fabulously misleading (which apparently triggered the comment change in
the first place), I'd consider it important to fix the comment.

It might even be an idea to replace the condition with

    if (SCM_IMP (key) || !SCM_NUMBERP (key))

(which yields an ugly macro expansion but the compiler might optimize
it) or

    if (SCM_IMP (key) || !SCM_NUMP (key))

both of which are sort of nonsensical to the compiler (the latter more
so than the former) but might be less confusing to the human reader.

Or find a better name for SCM_NUMP in the first place.

At any rate, the minimally invasive option would be to restore the
original comment albeit with typographic fixes, namely writing `eq?'
instead of just eq.

-- 
David Kastrup