* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
@ 2013-03-22 1:24 Andreas Politz
2013-03-22 5:12 ` Drew Adams
0 siblings, 1 reply; 12+ messages in thread
From: Andreas Politz @ 2013-03-22 1:24 UTC (permalink / raw)
To: 14029
,----[ (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
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-03-22 1:24 bug#14029: 24.2.50; [PATCH] imenu problems with special elements Andreas Politz
@ 2013-03-22 5:12 ` Drew Adams
[not found] ` <87ip4iwfoe.fsf@fh-trier.de>
0 siblings, 1 reply; 12+ messages in thread
From: Drew Adams @ 2013-03-22 5:12 UTC (permalink / raw)
To: 'Andreas Politz', 14029
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).
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
[not found] ` <87ip4iwfoe.fsf@fh-trier.de>
@ 2013-03-23 15:54 ` Andreas Politz
2013-03-23 16:15 ` Drew Adams
2013-11-24 23:53 ` Dmitry Gutov
0 siblings, 2 replies; 12+ messages in thread
From: Andreas Politz @ 2013-03-23 15:54 UTC (permalink / raw)
To: 14029
Andreas Politz <politza@fh-trier.de> writes:
> "Drew Adams" <drew.adams@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-03-23 15:54 ` Andreas Politz
@ 2013-03-23 16:15 ` Drew Adams
2013-11-24 23:53 ` Dmitry Gutov
1 sibling, 0 replies; 12+ messages in thread
From: Drew Adams @ 2013-03-23 16:15 UTC (permalink / raw)
To: 'Andreas Politz', 14029
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-03-23 15:54 ` Andreas Politz
2013-03-23 16:15 ` Drew Adams
@ 2013-11-24 23:53 ` Dmitry Gutov
2013-11-25 1:32 ` Andreas Politz
2013-11-25 1:32 ` Andreas Politz
1 sibling, 2 replies; 12+ messages in thread
From: Dmitry Gutov @ 2013-11-24 23:53 UTC (permalink / raw)
To: Andreas Politz; +Cc: 14029
Hi there,
Andreas Politz <politza@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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-24 23:53 ` Dmitry Gutov
@ 2013-11-25 1:32 ` Andreas Politz
2013-11-25 1:40 ` Andreas Politz
2013-11-25 1:32 ` Andreas Politz
1 sibling, 1 reply; 12+ messages in thread
From: Andreas Politz @ 2013-11-25 1:32 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 14029
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: imenu.patch --]
[-- Type: text/x-diff, Size: 3583 bytes --]
=== 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@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@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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-24 23:53 ` Dmitry Gutov
2013-11-25 1:32 ` Andreas Politz
@ 2013-11-25 1:32 ` Andreas Politz
2013-11-29 3:43 ` Dmitry Gutov
1 sibling, 1 reply; 12+ messages in thread
From: Andreas Politz @ 2013-11-25 1:32 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 14029
[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: imenu.patch --]
[-- Type: text/x-diff, Size: 3580 bytes --]
=== 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@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@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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-25 1:32 ` Andreas Politz
@ 2013-11-25 1:40 ` Andreas Politz
0 siblings, 0 replies; 12+ messages in thread
From: Andreas Politz @ 2013-11-25 1:40 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 14029
Andreas Politz <politza@hochschule-trier.de> writes:
> (imenu--index-alist): Don't recurse into non-subalists.
This should be read as (imenu--in-alist).
-ap
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-25 1:32 ` Andreas Politz
@ 2013-11-29 3:43 ` Dmitry Gutov
2013-11-29 13:36 ` Andreas Politz
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Gutov @ 2013-11-29 3:43 UTC (permalink / raw)
To: Andreas Politz; +Cc: 14029-done
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-29 3:43 ` Dmitry Gutov
@ 2013-11-29 13:36 ` Andreas Politz
2013-11-29 14:35 ` Dmitry Gutov
2013-11-29 15:34 ` Drew Adams
0 siblings, 2 replies; 12+ messages in thread
From: Andreas Politz @ 2013-11-29 13:36 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 14029-done
[-- Attachment #1: Type: text/plain, Size: 831 bytes --]
Dmitry Gutov <dgutov@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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: imenu.patch --]
[-- Type: text/x-diff, Size: 701 bytes --]
=== 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-29 13:36 ` Andreas Politz
@ 2013-11-29 14:35 ` Dmitry Gutov
2013-11-29 15:34 ` Drew Adams
1 sibling, 0 replies; 12+ messages in thread
From: Dmitry Gutov @ 2013-11-29 14:35 UTC (permalink / raw)
To: Andreas Politz; +Cc: 14029-done
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#14029: 24.2.50; [PATCH] imenu problems with special elements
2013-11-29 13:36 ` Andreas Politz
2013-11-29 14:35 ` Dmitry Gutov
@ 2013-11-29 15:34 ` Drew Adams
1 sibling, 0 replies; 12+ messages in thread
From: Drew Adams @ 2013-11-29 15:34 UTC (permalink / raw)
To: Andreas Politz, Dmitry Gutov; +Cc: 14029-done
> >>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-29 15:34 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-22 1:24 bug#14029: 24.2.50; [PATCH] imenu problems with special elements Andreas Politz
2013-03-22 5:12 ` Drew Adams
[not found] ` <87ip4iwfoe.fsf@fh-trier.de>
2013-03-23 15:54 ` Andreas Politz
2013-03-23 16:15 ` Drew Adams
2013-11-24 23:53 ` Dmitry Gutov
2013-11-25 1:32 ` Andreas Politz
2013-11-25 1:40 ` Andreas Politz
2013-11-25 1:32 ` Andreas Politz
2013-11-29 3:43 ` Dmitry Gutov
2013-11-29 13:36 ` Andreas Politz
2013-11-29 14:35 ` Dmitry Gutov
2013-11-29 15:34 ` Drew Adams
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.