GNU bug report logs - #14029
24.2.50; [PATCH] imenu problems with special elements

Previous Next

Full log

View this message in rfc822 format

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Andreas Politz'" <politza <at> fh-trier.de>, <14029 <at> debbugs.gnu.org>
Subject: bug#14029: 24.2.50; [PATCH] imenu problems with special elements 
Date: Thu, 21 Mar 2013 22:12:34 -0700

Thanks for this report and fix.  Neither the original code nor your patch is
super clear to me, so I have some (non-rhetorical) questions below.  But if
someone else understands this better, feel free to ignore.

> | Special elements look like this:
> |           (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
> |      Selecting a special element performs:
> |           (funcall FUNCTION
> |                    INDEX-NAME INDEX-POSITION ARGUMENTS...)
> 
> 1. At least one function does not recognize these elements, resulting
> in an error.

Can you be more specific?  Which function?  What error?  Recipe to reproduce?
(Maybe you are referring to the last change in your patch, for `imenu'?)

> 2. The advertised calling convention is different from the 
> actual one in `imenu'.

How so?  What difference do you see, where?

> 3. The `imenu--subalist-p' predicate is unsafe and incomplete.

How so?

>   (defun imenu--subalist-p (item)
> !   (and (consp (cdr item)) (listp (cadr item))

   (defun imenu--subalist-p (item)
> !   (and (consp item)
> !        (consp (cdr item))
> !        (listp (cadr item))

(consp (cdr item)) is equivalent to
(and (consp item) (consp (cdr item))), assuming ITEM has the proper form (so
that cdr does not raise an error).  (consp (cdr-safe item)) should do the same
thing.

> !        (not (eq (car (cadr item)) 'lambda))))

> !        (not (functionp (cadr item)))))

Is it possible for (cadr item) to be a list and also be `functionp' and yet not
have its car be `lambda'?  Dunno.  I was under the impression that it was
impossible, but I could be wrong.  If it is possible, is it better to test
`functionp' here?  Dunno.

> ! 	     (if (setq res (imenu--in-alist str tail))

> ! 	     (if (and (imenu--subalist-p elt)
> !                 (setq res (imenu--in-alist str tail)))

Why is (imenu--subalist-p elt) needed here?  What error case does it prevent?  

Can't the code assume a properly constructed menu here, so that if TAIL is a
list then it is a bottom-level element, and so it is proper to just set ALIST to
nil?

> ! 	   (rest (if is-special-item (cddr index-item))))

> ! 	   (rest (if is-special-item (cdddr index-item))))

This change looks good, but `cdddr' is in cl.el, so perhaps it is better to use
(nthcdr 3 index-item).

I'm only a little bit surprised that this one hasn't already been reported and
fixed - there have been other bugs (e.g. #12717) related to special items.  I
use special items myself, but so far I have not used non-nil ARGS, so I have not
encountered this one (your last change).

Previous Next

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