Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

Dmitry Gutov <address@hidden> writes:

> Thank you, this is a considerable improvement. See comments below.
>
> On 08/11/2015 05:56 AM, Stephen Leake wrote:
>> branch: master
>> commit d7df36e745a5ba480559b6c8b5ebc93a18fe9bd1
>> Author: Stephen Leake <address@hidden>
>> Commit: Stephen Leake <address@hidden>
>>
>>      Always output all available definitions.
>
> How come you saw fit to undo the tweaks that I've added over time?

Because they got in the way of some of my uses of xref-find-definitions.

In general, rather than using similar heuristics in the low level code,
we should be adding hints from the source being searched, and/or the
user.

For example, one heuristic returned only the function when there are
both function and feature with the same name. But there are times when I
want to see both, or one or the other. So if I'm searching for the
identifier at point, in code like this:

        (dvc-log-edit ...) ;; point on '-l'; show defun
        (require 'dvc-log-edit) ;; point on '-l'; show feature

In both cases, we can easily tell from the source near point what the
user is searching for.

> In particular, now jumping to whitespace-mode will show two entries,
> even though they both lead to one location. That's a waste of time.

Yes. But I thought the heuristic the previous code used was: "if there
is both a variable and a function by the same name, _assume_ they are
located in the same place, so only return the function". That assumption
is broken for some of my code, and I assume in many others as well.

However, you point out later that you used (memq sym minor-mode-list) to
determine if this is from define-minor-mode.

I didn't realize why that was there when reading the code.

So I'll put that back, with a comment this time :).

>> -(defvar elisp--xref-format
>> +(defconst elisp--xref-format
>>     (let ((str "(%s %s)"))
>>       (put-text-property 1 3 'face 'font-lock-keyword-face str)
>>       (put-text-property 4 6 'face 'font-lock-function-name-face str)
>>       str))
>
> I'm not sure which part of the change did that, but now I don't see
> the colors in the output.
>
> Even though the approach is questionable, the result should be worth
> keeping.

I noticed that as well; I tracked it down. See bug#21237; if you use
defconst here, the text properties disappear when emacs is dumped.

So I've changed it back to defvar in my local code; it will be pushed
soon. 

>> +(defcustom find-feature-regexp
>
> I think these should still go into find-func.el. Even if you can't
> require it at the top of elisp-mode.el, you can do that at the
> beginning of elisp--xref-find-definitions. If that's actually needed.

The other one is find-alias-regexp.

Yes; then other code could use it as well. Should the default value of
find-function-regexp-alist contain them? I don't see any harm in that.

>> +       ((setq generic (cl--generic symbol))
>> +        (dolist (method (cl--generic-method-table generic))
>> +          (let* ((info (cl--generic-method-info method))
>> +                 (met-name (cons symbol (cl--generic-method-specializers 
>> method)))
>> + (descr (format elisp--xref-format-cl-defmethod 'cl-defmethod
>> symbol (nth 1 info)))
>> +                 (file (find-lisp-object-file-name met-name 'cl-defmethod)))
>
> This should also account, if possible, for the default method
> definitions performed by cl-defgeneric (and hide them).

Yes, I just ran across that. I'll add a test case. I'm not sure how to
detect the default methods, but maybe there is a way.

> For instance, looking for project-search-path will display three
> entries, but the last two both lead to its generic definition. 

The three are:

  (cl-defgeneric project-search-path)
  (cl-defmethod project-search-path (project))
  (cl-defmethod project-search-path ((project (head vc))))

the first shows the cl-defgeneric location.

the second os the default method (no specializer arg), but it shows the
cl-defmethod for (head vc); I'm not clear why.

the third shows the cl-defmethod for (head vc), as it should.

>> +;; When tests are run from the Makefile, 'default-directory' is $HOME,
>> +;; so we must provide this dir to expand-file-name in the expected
>> +;; results. The Makefile sets EMACS_TEST_DIRECTORY.
>> +(defconst emacs-test-dir (getenv "EMACS_TEST_DIRECTORY"))
>
> You could also use (or load-file-name (buffer-file-name)). That has
> the benefit of being usable when tests are run inside the interactive
> session. Not via Makefile.

Ah, that's a good idea. I have a "(setenv "EMACS_TEST_DIRECTORY")" in my
notes file for the iteractive case.

-- 
-- Stephe