Re: Generalizing find-definition

[Stefan Monnier]

> - find-definition does not store locations in the tag ring yet
>   (ran out of time and wanted to post the initial patch for feedback)

That needs to be written before it can be installed.

> - The whole "find uses" interface

That can be written later.

> - etags.el integration

That needs to be written before it can be installed.

> - emacs-lisp-mode integration

That can be written later.  It should probably obsolete
find-definition-noselect.

> This also does define a minor mode instead of changing the global key
> bindings.  I think in the final version it should replace the key
> binding definitions done in etags.el.  Is this correct?

Yes, that's correct.

> Do I need to hook it up elsewhere in the build process?

I don't think so.

Thanks, it looks pretty good.  See further comments below.


        Stefan


> +(defcustom find-definition-marker-ring-length 16
> +  "Length of marker rings `find-definition-marker-ring'."
> +  :group 'find-definition
> +  :type 'integer)

The :group is redundant.

> +(defvar find-definition-function nil
> +  "The function `find-definition' calls to find the definition.
> +
> +Will be called with no arguments with point at the location of
> +the thing to find the definition for. It should return a list
> +with each element being a list of one to three elements. The
> +first element should be the file name, the second the
> +line (defaulting to 1) and the third the column (defaulting to
> +0).")

Please use two spaces between sentences.  Also we usually prefer to
describe things using "patterns", as in:

   Will be called with no arguments with point at the location of
   the thing to find the definition for.  It should return a list
   of elements of the form (FILE LINE COL) where LINE and COL can be
   omitted.")

> +(defvar find-definition-identifier-function nil

I suggest you collapse those two function variables into one: if called
with a nil value (or without argument), then just find the definitions
of "thing at point" and if called with a string, then find the
definitions of that identifier.

> +  (let ((map (make-sparse-keymap)))
> +    (define-key map (kbd "M-.") 'find-definition)
> +    (define-key map (kbd "C-x 4 .") 'find-definition-other-window)
> +    (define-key map (kbd "C-x 5 .") 'find-definition-other-frame)
> +    ;; (define-key map (kbd "M-_") 'find-definition-uses)
> +    (define-key map (kbd "M-,") 'find-definition-goto-last-position)

We should probably keep a M-* binding for now.

> +;;;###autoload
> +(define-minor-mode find-definition-mode
> +  "Minor mode to provide some key bindings to find definitions.
> +\\{find-definition-mode-map}"
> +  :keymap 'find-definition-mode)
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I don't think this really does what you want.  Better just completely
remove it [ Not that it matters since find-definition-mode should
simply disappear.  ].

> +;;;###autoload
> +(defun find-definition (&optional ask)
> +  "Go to the definition of the thing at point.
> +If the definition can not be found, or with a prefix argument,
> +prompt for a symbol to use."
> +  (interactive "P")
> +  (switch-to-buffer (find-definition--noselect ask)))

Rather than ask from the body of the function, I think the prompting
would be better done in the interactive form.  I.e.

   (defun find-definition (&optional identifier)
     "Go to the definition of the thing at point.
   If the definition can not be found, or with a prefix argument,
   prompt for a symbol to use."
     (interactive
      (if current-prefix-arg (list (find-definition--read-identifier))))
     (switch-to-buffer (find-definition--noselect identifier)))

This of course also suggests we should not do the "if the definition can
not be found, prompt for a symbol to use".

> +      ;; Exactly one definition

Please punctuate your comments.

> +      (with-current-buffer outbuf
> +        (erase-buffer)
> +        (setq default-directory dir)
> +        (compilation-mode)
> +        (dolist (location locations)
> +          (let* ((filename (elt location 0))
> +                 (line (or (elt location 1)
> +                           1))
> +                 (col (or (elt location 2)
> +                          0))
> +                 (buffer (find-buffer-visiting filename))
> +                 (line-string
> +                  (when buffer
> +                    (with-current-buffer buffer
> +                      (save-excursion
> +                        (save-restriction
> +                          (widen)
> +                          (goto-char (point-min))
> +                          (forward-line (- line 1))
> +                          (buffer-substring (line-beginning-position)
> +                                            (line-end-position))))))))
> +            (insert (format "%s:%s:%s:%s\n"
> +                            filename line col
> +                            (or line-string
> +                                "")))))

We can probably use a new mode that derives from compilation-mode.
That will let us use a more efficient and reliable regexp to match the
entries we put in there.
Also, I think that when the LINE&COL are absent, we should not move to
LINE=1, but instead just leave point alone in that buffer (and in the
list, we should not explicitly say "FILE:1:0" but just "FILE:" or
something like that).

One remaining issue is with the filenames themselves: the
find-definition-function may very well (and maybe should usually) return
absolute file names, so we should "prettify" them (make them relative)
before displaying them in the *Definitions* buffer.


        Stefan