GNU bug report logs - #22630
[PATCH] Let assv/assoc shortcircuit to assq where feasible

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 22630 in the body.
You can then email your comments to 22630 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: David Kastrup <dak <at> gnu.org>
To: bug-guile <at> gnu.org
Cc: David Kastrup <dak <at> gnu.org>
Subject: [PATCH] Let assv/assoc shortcircuit to assq where feasible
Date: Thu, 11 Feb 2016 12:31:48 +0100

	* 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"
 
 
@@ -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

Message #10 received at 22630-done <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: David Kastrup <dak <at> gnu.org>
Cc: 22630-done <at> debbugs.gnu.org
Subject: Re: bug#22630: [PATCH] Let assv/assoc shortcircuit to assq where
 feasible
Date: Sun, 07 Aug 2016 22:53:41 +0200

On Thu 11 Feb 2016 12:31, David Kastrup <dak <at> 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

Message #17 received at 22630 <at> debbugs.gnu.org (full text, mbox):

From: David Kastrup <dak <at> gnu.org>
To: 22630 <at> debbugs.gnu.org
Subject: Bad comment
Date: Tue, 11 Oct 2016 11:03:11 +0200

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.