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

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 14029 in the body.
You can then email your comments to 14029 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: Andreas Politz <politza <at> fh-trier.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.2.50; [PATCH] imenu problems with special elements 
Date: Fri, 22 Mar 2013 02:24:33 +0100

,----[ (info "(elisp) Imenu") ]
| 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.

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

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

Andreas

diff -c -L /home/politza/src/emacs/trunk/lisp/imenu.el -L \#\<buffer\ imenu.el\> /home/politza/src/emacs/trunk/lisp/imenu.el /tmp/buffer-content-3095O-q
*** /home/politza/src/emacs/trunk/lisp/imenu.el
--- #<buffer imenu.el>
***************
*** 286,293 ****
  
  
  (defun imenu--subalist-p (item)
!   (and (consp (cdr item)) (listp (cadr item))
!        (not (eq (car (cadr item)) 'lambda))))
  
  (defmacro imenu-progress-message (_prevpos &optional _relpos _reverse)
    "Macro to display a progress message.
--- 286,295 ----
  
  
  (defun imenu--subalist-p (item)
!   (and (consp item)
!        (consp (cdr item))
!        (listp (cadr item))
!        (not (functionp (cadr item)))))
  
  (defmacro imenu-progress-message (_prevpos &optional _relpos _reverse)
    "Macro to display a progress message.
***************
*** 641,647 ****
        ;; We are only interested in the bottom-level elements, so we need to
        ;; recurse if TAIL is a list.
        (cond ((listp tail)
! 	     (if (setq res (imenu--in-alist str tail))
  		 (setq alist nil)))
  	    ((if imenu-name-lookup-function
                   (funcall imenu-name-lookup-function str head)
--- 643,650 ----
        ;; We are only interested in the bottom-level elements, so we need to
        ;; recurse if TAIL is a list.
        (cond ((listp tail)
! 	     (if (and (imenu--subalist-p elt)
!                       (setq res (imenu--in-alist str tail)))
  		 (setq alist nil)))
  	    ((if imenu-name-lookup-function
                   (funcall imenu-name-lookup-function str head)
***************
*** 1024,1030 ****
  		(nth 2 index-item) imenu-default-goto-function))
  	   (position (if is-special-item
  			 (cadr index-item) (cdr index-item)))
! 	   (rest (if is-special-item (cddr index-item))))
        (apply function (car index-item) position rest))
      (run-hooks 'imenu-after-jump-hook)))
  
--- 1027,1033 ----
  		(nth 2 index-item) imenu-default-goto-function))
  	   (position (if is-special-item
  			 (cadr index-item) (cdr index-item)))
! 	   (rest (if is-special-item (cdddr index-item))))
        (apply function (car index-item) position rest))
      (run-hooks 'imenu-after-jump-hook)))
  

Diff finished.  Fri Mar 22 02:15:09 2013

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

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Andreas Politz'" <politza <at> fh-trier.de>, <14029 <at> debbugs.gnu.org>
Subject: RE: 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).

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

From: Andreas Politz <politza <at> fh-trier.de>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Sat, 23 Mar 2013 16:54:58 +0100

Andreas Politz <politza <at> fh-trier.de> writes:

> "Drew Adams" <drew.adams <at> oracle.com> writes:
>
>> 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'?)
>
> Yes in `imenu--in-alist'.  It used to be >1, but one is already fixed in
> bzr.
>
>>
>>> 2. The advertised calling convention is different from the 
>>> actual one in `imenu'.
>>
>> How so?  What difference do you see, where?
>>
>
> The last change in `imenu' about cdddr.  
>
>>> 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.
>
> That's right.
>
>>
>>> !        (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 the documentation states FUNCTION, then it should be a function.
>
> Let's recap.  The three types are:
>
> (INDEX-NAME . INDEX-POSITION)
> (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
> (MENU-TITLE SUB-ALIST)
>
> First, I think the documentation is incorrect, and the last one should read
>
> (MENU-TITLE . SUB-ALIST)
>
> since SUB-ALIST is supposed to be the cdr of the list (see
> e.g. imenu--split-submenus)
>
> So (listp (cdr item)) would exclude the first type and we are
> left with
>
> (INDEX-POSITION FUNCTION ARGUMENTS...) and
> SUB-ALIST = (ITEM ITEM ...)
>
> and (cadr item) is either a function, an item or nil.  I think
> that INDEX-NAME and MENU-TITLE (the car of a possible item) are
> supposed to be strings, so this should work.
>
>
>>
>>> ! 	     (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?
>>
>
> No, it may contain subalists.
>
>>> ! 	   (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).
>
> That, and you probably always used the mouse menu.  
>
> A

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

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

Ok, thanks for the reply.  I don't have time to look into it further - I mainly
wanted you to clarify and double-check.

Please consider adding some comments to the code to help understanding (e.g. wrt
the fixed code).  Both Stefan and I have mentioned that the code is not too
clear on its own, and I imagine that you feel the same: it could be clearer with
a couple of comments.  Thx.

>> 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).
>
> That, and you probably always used the mouse menu. 

Actually, I use the keyboard, but my use of special items is limited to adding
two special menu items (that toggle sorting and toggle case-sensitive sorting).

FWIW, from the keyboard I use the commands described here:
http://www.emacswiki.org/emacs/Icicles_-_Other_Search_Commands#IciclesImenu.
They let you quickly choose among definitions of a given type, matching either
the object name or its full definition.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Politz <politza <at> fh-trier.de>
Cc: 14029 <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Mon, 25 Nov 2013 01:53:09 +0200

Hi there,

Andreas Politz <politza <at> fh-trier.de> writes:

>> Let's recap.  The three types are:
>>
>> (INDEX-NAME . INDEX-POSITION)
>> (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
>> (MENU-TITLE SUB-ALIST)
>>
>> First, I think the documentation is incorrect, and the last one should read
>>
>> (MENU-TITLE . SUB-ALIST)
>>
>> since SUB-ALIST is supposed to be the cdr of the list (see
>> e.g. imenu--split-submenus)

Is this documentation fix supposed to be included in the patch? I only
see source code changes in it.

Aside from that, you should consider including a proper ChangeLog entry
in the patch (with detailed descriptions, as is the custom).  Without
it and a reproduction recipe, I'm having hard time understanding what
change does what, too.

The patchy description in email replies doesn't really cut it.  Also
note that your last reply is one huge quote.

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

From: Andreas Politz <politza <at> hochschule-trier.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 14029 <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Mon, 25 Nov 2013 02:32:33 +0100

[Message part 1 (text/plain, inline)]

Changelog:

* imenu.el (imenu--subalist-p): Don't error on non-conses and
allow non-lambda lists as functions.

In my opinion a predicate should not throw an error.  Anyway more
important is the `non-lambda' lists part.

(imenu--index-alist): Don't recurse into non-subalists.

Looking at the commentary, the ability to embed functions in a menu has
clearly been added after this functions was written.

(imenu): Don't pass function itself as an argument.

,----[ C-h v (describe-variable 'imenu-generic-expression) RET ]
| Each element of this list should have the form
| 
|   (MENU-TITLE REGEXP INDEX [FUNCTION] [ARGUMENTS...])
| 
| FUNCTION, if present, specifies a function to call when the index
| item is selected by the user.  This function is called with
| arguments consisting of the item name, the buffer position, and
| the ARGUMENTS.
`----

(cddr index-item) == (FUNCTION . ARGS), is wrong as argument list, we
need one more cdr.

* doc/lispref/modes.texi: Make it clear that sub-alist is the cdr.

-ap

[imenu.patch (text/x-diff, inline)]

=== modified file 'doc/lispref/modes.texi'
*** doc/lispref/modes.texi	2013-08-17 11:14:10 +0000
--- doc/lispref/modes.texi	2013-11-25 00:39:51 +0000
***************
*** 2483,2489 ****
  A nested sub-alist element looks like this:
  
  @example
! (@var{menu-title} @var{sub-alist})
  @end example
  
  It creates the submenu @var{menu-title} specified by @var{sub-alist}.
--- 2483,2489 ----
  A nested sub-alist element looks like this:
  
  @example
! (@var{menu-title} . @var{sub-alist})
  @end example
  
  It creates the submenu @var{menu-title} specified by @var{sub-alist}.

=== modified file 'lisp/ChangeLog'
*** lisp/ChangeLog	2013-11-24 22:53:35 +0000
--- lisp/ChangeLog	2013-11-25 00:45:59 +0000
***************
*** 1,3 ****
--- 1,10 ----
+ 2013-11-22  Andreas Politz  <politza <at> fh-trier.de>
+ 	* imenu.el (imenu--subalist-p): Don't error on non-conses and
+ 	allow non-lambda lists as functions.
+ 	(imenu--index-alist): Don't recurse into non-subalists.
+ 	(imenu): Don't pass function itself as an argument.
+ 	* doc/lispref/modes.texi: Make it clear that sub-alist is the cdr.
+ 
  2013-11-24  Simon Schubert  <2 <at> 0x2c.org>  (tiny change)
  
  	* json.el (json-alist-p): Only return non-nil if the alist has

=== modified file 'lisp/imenu.el'
*** lisp/imenu.el	2013-11-24 21:23:47 +0000
--- lisp/imenu.el	2013-11-25 01:26:34 +0000
***************
*** 293,300 ****
  
  
  (defun imenu--subalist-p (item)
!   (and (consp (cdr item)) (listp (cadr item))
!        (not (eq (car (cadr item)) 'lambda))))
  
  (defmacro imenu-progress-message (_prevpos &optional _relpos _reverse)
    "Macro to display a progress message.
--- 293,302 ----
  
  
  (defun imenu--subalist-p (item)
!   (and (consp item)
!        (consp (cdr item))
!        (listp (cadr item))
!        (not (functionp (cadr item)))))
  
  (defmacro imenu-progress-message (_prevpos &optional _relpos _reverse)
    "Macro to display a progress message.
***************
*** 645,653 ****
        ;;   (INDEX-NAME (INDEX-NAME . INDEX-POSITION) ...)
        ;; while a bottom-level element looks like
        ;;   (INDEX-NAME . INDEX-POSITION)
        ;; We are only interested in the bottom-level elements, so we need to
!       ;; recurse if TAIL is a list.
!       (cond ((listp tail)
  	     (if (setq res (imenu--in-alist str tail))
  		 (setq alist nil)))
  	    ((if imenu-name-lookup-function
--- 647,657 ----
        ;;   (INDEX-NAME (INDEX-NAME . INDEX-POSITION) ...)
        ;; while a bottom-level element looks like
        ;;   (INDEX-NAME . INDEX-POSITION)
+       ;;   or
+       ;;   (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
        ;; We are only interested in the bottom-level elements, so we need to
!       ;; recurse if TAIL is a nested ALIST.
!       (cond ((imenu--subalist-p elt)
  	     (if (setq res (imenu--in-alist str tail))
                   (setq alist nil)))
  	    ((if imenu-name-lookup-function
***************
*** 1033,1040 ****
  		(nth 2 index-item) imenu-default-goto-function))
  	   (position (if is-special-item
  			 (cadr index-item) (cdr index-item)))
! 	   (rest (if is-special-item (cddr index-item))))
!       (apply function (car index-item) position rest))
      (run-hooks 'imenu-after-jump-hook)))
  
  (provide 'imenu)
--- 1037,1044 ----
  		(nth 2 index-item) imenu-default-goto-function))
  	   (position (if is-special-item
  			 (cadr index-item) (cdr index-item)))
! 	   (args (if is-special-item (cdr (cddr index-item)))))
!       (apply function (car index-item) position args))
      (run-hooks 'imenu-after-jump-hook)))
  
  (provide 'imenu)

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

From: Andreas Politz <politza <at> hochschule-trier.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 14029 <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Mon, 25 Nov 2013 02:40:47 +0100

Andreas Politz <politza <at> hochschule-trier.de> writes:

> (imenu--index-alist): Don't recurse into non-subalists.

This should be read as (imenu--in-alist).

-ap

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

From: Andreas Politz <politza <at> hochschule-trier.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 14029 <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Mon, 25 Nov 2013 02:32:33 +0100

[Message part 1 (text/plain, inline)]

Let's do that again.

Changelog:

* imenu.el (imenu--subalist-p): Don't error on non-conses and
allow non-lambda lists as functions.

In my opinion a predicate should not throw an error.  Anyway more
important is the `non-lambda' lists part.

(imenu--in-alist): Don't recurse into non-subalists.

Looking at the commentary, the ability to embed functions in a menu has
clearly been added after this functions was written.

(imenu): Don't pass function itself as an argument.

,----[ (info "(elisp) Imenu") ]
|      Selecting a simple element has the effect of moving to position
|      INDEX-POSITION in the buffer.  Special elements look like this:
| 
|           (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
| 
|      Selecting a special element performs:
| 
|           (funcall FUNCTION
|                    INDEX-NAME INDEX-POSITION ARGUMENTS...)
`----

(cddr index-item) == (FUNCTION . ARGS), is wrong as argument list, we
need one more cdr.

* doc/lispref/modes.texi: Make it clear that sub-alist is the cdr.

-ap

[imenu.patch (text/x-diff, inline)]

=== modified file 'doc/lispref/modes.texi'
*** doc/lispref/modes.texi	2013-08-17 11:14:10 +0000
--- doc/lispref/modes.texi	2013-11-25 00:39:51 +0000
***************
*** 2483,2489 ****
  A nested sub-alist element looks like this:
  
  @example
! (@var{menu-title} @var{sub-alist})
  @end example
  
  It creates the submenu @var{menu-title} specified by @var{sub-alist}.
--- 2483,2489 ----
  A nested sub-alist element looks like this:
  
  @example
! (@var{menu-title} . @var{sub-alist})
  @end example
  
  It creates the submenu @var{menu-title} specified by @var{sub-alist}.

=== modified file 'lisp/ChangeLog'
*** lisp/ChangeLog	2013-11-24 22:53:35 +0000
--- lisp/ChangeLog	2013-11-25 00:45:59 +0000
***************
*** 1,3 ****
--- 1,10 ----
+ 2013-11-22  Andreas Politz  <politza <at> fh-trier.de>
+ 	* imenu.el (imenu--subalist-p): Don't error on non-conses and
+ 	allow non-lambda lists as functions.
+ 	(imenu--in-alist): Don't recurse into non-subalists.
+ 	(imenu): Don't pass function itself as an argument.
+ 	* doc/lispref/modes.texi: Make it clear that sub-alist is the cdr.
+ 
  2013-11-24  Simon Schubert  <2 <at> 0x2c.org>  (tiny change)
  
  	* json.el (json-alist-p): Only return non-nil if the alist has

=== modified file 'lisp/imenu.el'
*** lisp/imenu.el	2013-11-24 21:23:47 +0000
--- lisp/imenu.el	2013-11-25 01:26:34 +0000
***************
*** 293,300 ****
  
  
  (defun imenu--subalist-p (item)
!   (and (consp (cdr item)) (listp (cadr item))
!        (not (eq (car (cadr item)) 'lambda))))
  
  (defmacro imenu-progress-message (_prevpos &optional _relpos _reverse)
    "Macro to display a progress message.
--- 293,302 ----
  
  
  (defun imenu--subalist-p (item)
!   (and (consp item)
!        (consp (cdr item))
!        (listp (cadr item))
!        (not (functionp (cadr item)))))
  
  (defmacro imenu-progress-message (_prevpos &optional _relpos _reverse)
    "Macro to display a progress message.
***************
*** 645,653 ****
        ;;   (INDEX-NAME (INDEX-NAME . INDEX-POSITION) ...)
        ;; while a bottom-level element looks like
        ;;   (INDEX-NAME . INDEX-POSITION)
        ;; We are only interested in the bottom-level elements, so we need to
!       ;; recurse if TAIL is a list.
!       (cond ((listp tail)
  	     (if (setq res (imenu--in-alist str tail))
  		 (setq alist nil)))
  	    ((if imenu-name-lookup-function
--- 647,657 ----
        ;;   (INDEX-NAME (INDEX-NAME . INDEX-POSITION) ...)
        ;; while a bottom-level element looks like
        ;;   (INDEX-NAME . INDEX-POSITION)
+       ;;   or
+       ;;   (INDEX-NAME INDEX-POSITION FUNCTION ARGUMENTS...)
        ;; We are only interested in the bottom-level elements, so we need to
!       ;; recurse if TAIL is a nested ALIST.
!       (cond ((imenu--subalist-p elt)
  	     (if (setq res (imenu--in-alist str tail))
                   (setq alist nil)))
  	    ((if imenu-name-lookup-function
***************
*** 1033,1040 ****
  		(nth 2 index-item) imenu-default-goto-function))
  	   (position (if is-special-item
  			 (cadr index-item) (cdr index-item)))
! 	   (rest (if is-special-item (cddr index-item))))
!       (apply function (car index-item) position rest))
      (run-hooks 'imenu-after-jump-hook)))
  
  (provide 'imenu)
--- 1037,1044 ----
  		(nth 2 index-item) imenu-default-goto-function))
  	   (position (if is-special-item
  			 (cadr index-item) (cdr index-item)))
! 	   (args (if is-special-item (cdr (cddr index-item)))))
!       (apply function (car index-item) position args))
      (run-hooks 'imenu-after-jump-hook)))
  
  (provide 'imenu)

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Politz <politza <at> hochschule-trier.de>
Cc: 14029-done <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Fri, 29 Nov 2013 05:43:57 +0200

Version: 24.4

Thanks, that's clearer. Installed in revision 115279.

One note:

>> 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 the documentation states FUNCTION, then it should be a function.

The documentation allows FUNCTION in the third element, but 
`imenu--subalist-p' is checking whether the second element is a function.

AFAICT, it's checking against malformed items. Your change makes it also 
guard against lexically-bound lambdas (their car is `closure'), so 
that's good, I guess.

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

From: Andreas Politz <politza <at> hochschule-trier.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 14029-done <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Fri, 29 Nov 2013 14:36:48 +0100

[Message part 1 (text/plain, inline)]

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> One note:
>
>>> Is it possible for (cadr item) to be a list and also be `functionp' and yet not
>>> have its car be `lambda'?
>
> The documentation allows FUNCTION in the third element, but
> imenu--subalist-p' is checking whether the second element is a
> function.

Yes, this doesn't make much sense.  I guess this function check was
intended to distinguish a special element

(INDEX-NAME POS FN . ARGS)

from a sub-alist element

(INDEX-NAME . SUB-ALIST).

The check would make sense, if this function was applied to the cdr of
an element, i.e. check if the argument is a SUB-ALIST.  But this is not
how this function is used in imenu.el .

I might have initially (and falsely) determined this as the source of
some bug.

-ap

P.S.: There is another dot missing in the documentation.

[imenu.patch (text/x-diff, inline)]

=== modified file 'lisp/imenu.el'
*** lisp/imenu.el	2013-11-29 03:38:20 +0000
--- lisp/imenu.el	2013-11-29 13:30:58 +0000
***************
*** 463,469 ****
  To \"go to\" a special element means applying FUNCTION
  to INDEX-NAME, POSITION, and the ARGUMENTS.
  
! A nested sub-alist element looks like (INDEX-NAME SUB-ALIST).
  The function `imenu--subalist-p' tests an element and returns t
  if it is a sub-alist.
  
--- 463,469 ----
  To \"go to\" a special element means applying FUNCTION
  to INDEX-NAME, POSITION, and the ARGUMENTS.
  
! A nested sub-alist element looks like (INDEX-NAME . SUB-ALIST).
  The function `imenu--subalist-p' tests an element and returns t
  if it is a sub-alist.
  

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Politz <politza <at> hochschule-trier.de>
Cc: 14029-done <at> debbugs.gnu.org
Subject: Re: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Fri, 29 Nov 2013 16:35:46 +0200

On 29.11.2013 15:36, Andreas Politz wrote:
> The check would make sense, if this function was applied to the cdr of
> an element, i.e. check if the argument is a SUB-ALIST.  But this is not
> how this function is used in imenu.el .

Yep, it's either a mistake or an example of defensive programming.

> I might have initially (and falsely) determined this as the source of
> some bug.

That's why we usually ask that bug reports have reproduction scenario.

> P.S.: There is another dot missing in the documentation.

Installed, thanks.

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Andreas Politz <politza <at> hochschule-trier.de>, Dmitry Gutov
 <dgutov <at> yandex.ru>
Cc: 14029-done <at> debbugs.gnu.org
Subject: RE: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Fri, 29 Nov 2013 07:34:19 -0800 (PST)

> >>> Is it possible for (cadr item) to be a list and also be `functionp' and
> >>> yet not have its car be `lambda'?
> >
> > The documentation allows FUNCTION in the third element, but
> > imenu--subalist-p' is checking whether the second element is a
> > function.
> 
> Yes, this doesn't make much sense.  I guess this function check was
> intended to distinguish a special element (INDEX-NAME POS FN . ARGS)
> from a sub-alist element (INDEX-NAME . SUB-ALIST).
> 
> The check would make sense, if this function was applied to the cdr of
> an element, i.e. check if the argument is a SUB-ALIST.  But this is not
> how this function is used in imenu.el .
> 
> I might have initially (and falsely) determined this as the source of
> some bug.

Sorry, I have not been following this thread at all, and am unaware
of the problem being addressed.  So please ignore, if this is irrelevant.

Emacs itself still does not use "special" items anywhere.  But I do.
I'm just hoping that they are still going to be taken into
consideration and not obliterated.  See bug #12717 for background on
submenus and special items.  Yes, the imenu.el code is a bit confusing.

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=12717

Previous Next

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