From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Generalizing find-definition Date: Tue, 18 Nov 2014 11:31:29 -0500 Message-ID: References: <20141102151524.0d9c665c@forcix> <20141117211039.37f03409@forcix> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1416328331 30784 80.91.229.3 (18 Nov 2014 16:32:11 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 18 Nov 2014 16:32:11 +0000 (UTC) Cc: emacs-devel@gnu.org To: Jorgen Schaefer Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Nov 18 17:32:04 2014 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Xqlh5-00009v-5O for ged-emacs-devel@m.gmane.org; Tue, 18 Nov 2014 17:31:55 +0100 Original-Received: from localhost ([::1]:54070 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqlh4-0001uJ-PA for ged-emacs-devel@m.gmane.org; Tue, 18 Nov 2014 11:31:54 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:45594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqlgt-0001tT-Pp for emacs-devel@gnu.org; Tue, 18 Nov 2014 11:31:51 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xqlgk-0003GH-UP for emacs-devel@gnu.org; Tue, 18 Nov 2014 11:31:43 -0500 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:34498) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqlgk-0003Fz-QD for emacs-devel@gnu.org; Tue, 18 Nov 2014 11:31:34 -0500 Original-Received: from pastel.home (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id sAIGVUpu003102; Tue, 18 Nov 2014 11:31:30 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 9DEAE7AC0; Tue, 18 Nov 2014 11:31:29 -0500 (EST) In-Reply-To: <20141117211039.37f03409@forcix> (Jorgen Schaefer's message of "Mon, 17 Nov 2014 21:10:39 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV5129=0 X-NAI-Spam-Version: 2.3.0.9393 : core <5129> : inlines <1536> : streams <1344638> : uri <1835181> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.22 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:177578 Archived-At: > - 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