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