From: Kaushal Modi <kaushal.modi@gmail.com>
To: npostavs@users.sourceforge.net
Cc: 24057@debbugs.gnu.org
Subject: bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path
Date: Fri, 17 Mar 2017 22:16:07 +0000 [thread overview]
Message-ID: <CAFyQvY11Hk=3Le=YfLgfGSCXTiVGHjJjNFihQAJ1Yi_kx3BUhw@mail.gmail.com> (raw)
In-Reply-To: <874lys64bm.fsf@users.sourceforge.net>
[-- Attachment #1.1: Type: text/plain, Size: 3139 bytes --]
Hi Noam,
Thanks for the detailed review. My comments are inline below.
Also I have attached a new patch re-based to the current master, and
applying all your suggestions.
On Thu, Mar 16, 2017 at 10:09 PM <npostavs@users.sourceforge.net> wrote:
> Kaushal Modi <kaushal.modi@gmail.com> writes:
>
> > * lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
> > ensure that the returned string does not contain the comment start
> > characters (especially for major modes that have '//' as comment start
> > characters).
> >
>
> Why is there a blank line here?
>
Is it is convention to not format the commit summaries into multiple
paragraphs? I reread
http://git.savannah.gnu.org/cgit/emacs.git/plain/CONTRIBUTE but didn't find
any mention on that. But I have removed that newline from the patch that I
attach with this email.
> > Otherwise, in a major mode like c-mode, with `ido-mode' enabled and
> > `ido-use-filename-at-point' set to `guess', doing "C-x C-f" on a "//foo"
> > comment will initiate an attempt to access a path "//foo" (Bug#24057).
> > ---
>
> > + ;; (message "ffap-string-at-point dbg: beg = %d end = %d" beg end)
>
> This can be removed.
>
Understood, the patch was still not in final stages.. The attached patch
does not have any debug statements.
> > + ;; Check if END character is part of a comment.
> > + (save-excursion
> > + (goto-char end)
> > + (nth 4 (syntax-ppss))))
>
> This could be just (nth 4 (syntax-ppss end)).
>
Thanks. TIL that syntax-ppss as POS as optional arg. Also retaining the
save-excursion as you correct yourself in the followup email.
> > + (save-excursion
> > + ;; Increment BEG till point at BEG is in a comment too.
> > + ;; (nth 4 (syntax-ppss)) will be null for comment start
> > + ;; characters (for example, for the "//" characters in
> > + ;; `c-mode' line comments).
> > + (setq beg (catch 'break
> > + (while (< beg-new end)
> > + (goto-char beg-new)
> > + (if (nth 4 (syntax-ppss)) ; in a comment
> > + (throw 'break beg-new)
> > + (setq beg-new (1+ beg-new))))
> > + end)))) ; Set BEG to END if no throw happens
>
> This could be just
>
> ;; Move BEG to beginning of comment (after the comment start
> ;; characters), or END, whichever comes first.
> (let ((state (syntax-ppss beg)))
> (unless (nth 4 state)
> (parse-partial-sexp beg end nil nil state t)
> (setq beg (point))))
>
Thanks! I did not know about parse-partial-sexp. Here too, I need to retain
the save-excursion, else the point will move after the comment start chars
if it is at the BOL in c-mode on a line like
//tmp
> > + ;; (message "ffap-string-at-point dbg: beg = %d beg-new = %d"
> > + ;; beg beg-new)
>
> > + ;; (message "ffap-string-at-point dbg: ffap-string-at-point = %S"
> > + ;; ffap-string-at-point)
>
> These can be removed.
>
Done!
Review review the attached. Thanks!
--
Kaushal Modi
[-- Attachment #1.2: Type: text/html, Size: 5788 bytes --]
[-- Attachment #2: 0001-Do-not-include-comment-start-chars-in-ffap-string.patch --]
[-- Type: application/octet-stream, Size: 3852 bytes --]
From eda7a96b66f6937e094b0d76364c6711984db254 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Fri, 17 Mar 2017 18:03:23 -0400
Subject: [PATCH] Do not include comment start chars in ffap string
* lisp/ffap.el (ffap-string-at-point): If the point is in a comment,
ensure that the returned string does not contain the comment start
characters (especially for major modes that have '//' as comment start
characters). Otherwise, in a major mode like c-mode, with `ido-mode'
enabled and `ido-use-filename-at-point' set to `guess', doing "C-x
C-f" on a "//foo" comment will initiate an attempt to access a path
"//foo" (Bug#24057).
Co-authored-by: Noam Postavsky <npostavs@gmail.com>
---
lisp/ffap.el | 41 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/lisp/ffap.el b/lisp/ffap.el
index d7222bfb68..a15083f4fa 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1110,32 +1110,67 @@ ffap-string-at-point
(defun ffap-string-at-point (&optional mode)
"Return a string of characters from around point.
+
MODE (defaults to value of `major-mode') is a symbol used to look up
string syntax parameters in `ffap-string-at-point-mode-alist'.
+
If MODE is not found, we use `file' instead of MODE.
+
If the region is active, return a string from the region.
-Set the variable `ffap-string-at-point' and the variable
+
+If the point is in a comment, ensure that the returned string does not
+contain the comment start characters (especially for major modes that
+have '//' as comment start characters).
+
+Set the variables `ffap-string-at-point' and
`ffap-string-at-point-region'.
+
When the region is active and larger than `ffap-max-region-length',
return an empty string, and set `ffap-string-at-point-region' to '(1 1)."
(let* ((args
(cdr
(or (assq (or mode major-mode) ffap-string-at-point-mode-alist)
(assq 'file ffap-string-at-point-mode-alist))))
+ (region-selected (use-region-p))
(pt (point))
- (beg (if (use-region-p)
+ (beg (if region-selected
(region-beginning)
(save-excursion
(skip-chars-backward (car args))
(skip-chars-forward (nth 1 args) pt)
(point))))
- (end (if (use-region-p)
+ (end (if region-selected
(region-end)
(save-excursion
(skip-chars-forward (car args))
(skip-chars-backward (nth 2 args) pt)
(point))))
(region-len (- (max beg end) (min beg end))))
+
+ ;; If the initial characters of the to-be-returned string are the
+ ;; current major mode's comment starter characters, *and* are
+ ;; not part of a comment, remove those from the returned string
+ ;; (Bug#24057).
+ ;; Example comments in `c-mode' (which considers lines beginning
+ ;; with "//" as comments):
+ ;; //tmp - This is a comment. It does not contain any path reference.
+ ;; ///tmp - This is a comment. The "/tmp" portion in that is a path.
+ ;; ////tmp - This is a comment. The "//tmp" portion in that is a path.
+ (when (and
+ ;; Proceed if no region is selected by the user.
+ (null region-selected)
+ ;; Check if END character is part of a comment.
+ (save-excursion
+ (nth 4 (syntax-ppss end))))
+ ;; Move BEG to beginning of comment (after the comment start
+ ;; characters), or END, whichever comes first.
+ (save-excursion
+ (let ((state (syntax-ppss beg)))
+ ;; (nth 4 (syntax-ppss)) will be nil for comment start chars
+ (unless (nth 4 state)
+ (parse-partial-sexp beg end nil nil state :commentstop)
+ (setq beg (point))))))
+
(if (and (natnump ffap-max-region-length)
(< region-len ffap-max-region-length)) ; Bug#25243.
(setf ffap-string-at-point-region (list beg end)
--
2.11.0
next prev parent reply other threads:[~2017-03-17 22:16 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-22 23:00 bug#24057: 25.1.50; ffap interprets comments beginning with "//" as file path Kaushal Modi
2016-07-22 23:11 ` Kaushal Modi
2016-07-23 1:26 ` npostavs
2016-07-23 7:38 ` Eli Zaretskii
2016-07-23 7:34 ` Eli Zaretskii
2016-07-23 11:56 ` Kaushal Modi
2016-07-23 13:05 ` Eli Zaretskii
2016-07-23 13:23 ` Kaushal Modi
2016-07-23 13:59 ` Eli Zaretskii
2016-07-23 18:02 ` Noam Postavsky
2016-07-23 18:20 ` Eli Zaretskii
2016-07-23 18:22 ` Eli Zaretskii
2016-07-23 21:51 ` Kaushal Modi
2016-07-24 14:16 ` Eli Zaretskii
2016-07-25 2:19 ` Kaushal Modi
2016-07-25 17:02 ` Eli Zaretskii
2016-07-25 17:13 ` Kaushal Modi
2016-07-25 17:24 ` Eli Zaretskii
2016-07-25 18:13 ` Kaushal Modi
2016-07-25 20:18 ` Kaushal Modi
2016-07-26 14:41 ` Eli Zaretskii
2016-07-26 15:11 ` Kaushal Modi
2017-03-17 2:10 ` npostavs
2017-03-17 2:13 ` npostavs
2017-03-17 22:16 ` Kaushal Modi [this message]
2017-03-17 23:30 ` npostavs
2017-03-18 1:28 ` Kaushal Modi
2017-03-18 15:41 ` npostavs
2017-03-23 13:01 ` npostavs
2017-03-23 13:30 ` Kaushal Modi
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
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFyQvY11Hk=3Le=YfLgfGSCXTiVGHjJjNFihQAJ1Yi_kx3BUhw@mail.gmail.com' \
--to=kaushal.modi@gmail.com \
--cc=24057@debbugs.gnu.org \
--cc=npostavs@users.sourceforge.net \
/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 public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).