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
