From: Andreas Politz <politza@fh-trier.de>
To: 14029@debbugs.gnu.org
Subject: bug#14029: 24.2.50; [PATCH] imenu problems with special elements
Date: Sat, 23 Mar 2013 16:54:58 +0100 [thread overview]
Message-ID: <878v5ew1pp.fsf@fh-trier.de> (raw)
In-Reply-To: <87ip4iwfoe.fsf@fh-trier.de> (Andreas Politz's message of "Sat, 23 Mar 2013 11:53:21 +0100")
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
next prev parent reply other threads:[~2013-03-23 15:54 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=878v5ew1pp.fsf@fh-trier.de \
--to=politza@fh-trier.de \
--cc=14029@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.