bug#14303: 24.3; Bug in comment-search-backward

[Leo Liu]

On 2013-04-29 21:27 +0800, Leo Liu wrote:
> Open a new buffer in octave-mode and insert the following line:
>
> x="#abc"
>
> Move point to the end of the inserted line and
>
> M-: (comment-search-backward)
>
> this moves point inside the string.

So it is no longer fine for comment-search-backward to end up in a
comment or string any more.

How about something like this?

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index d55feaa3..65182c1b 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -485,27 +485,25 @@ (defun comment-search-backward (&optional limit noerror)
Moves point to inside the comment and returns the position of the
comment-starter. If no comment is found, moves point to LIMIT
and raises an error or returns nil if NOERROR is non-nil."
- ;; FIXME: If a comment-start appears inside a comment, we may erroneously
- ;; stop there. This can be rather bad in general, but since
- ;; comment-search-backward is only used to find the comment-column (in
- ;; comment-set-column) and to find the comment-start string (via
- ;; comment-beginning) in indent-new-comment-line, it should be harmless.
- (if (not (re-search-backward comment-start-skip limit t))
- (unless noerror (error "No comment"))
- (beginning-of-line)
- (let* ((end (match-end 0))
- (cs (comment-search-forward end t))
- (pt (point)))
- (if (not cs)
- (progn (beginning-of-line)
- (comment-search-backward limit noerror))
- (while (progn (goto-char cs)
- (comment-forward)
- (and (< (point) end)
- (setq cs (comment-search-forward end t))))
- (setq pt (point)))
- (goto-char pt)
- cs))))
+ (let (found end)
+ (while (and (not found) (re-search-backward comment-start-skip limit t))
+ (setq end (match-end 0))
+ (or (nth 8 (syntax-ppss)) (setq found t)))
+ (if (not found)
+ (unless noerror (error "No comment"))
+ (beginning-of-line)
+ (let ((cs (comment-search-forward end t))
+ (pt (point)))
+ (if (not cs)
+ (progn (beginning-of-line)
+ (comment-search-backward limit noerror))
+ (while (progn (goto-char cs)
+ (comment-forward)
+ (and (< (point) end)
+ (setq cs (comment-search-forward end t))))
+ (setq pt (point)))
+ (goto-char pt)
+ cs)))))

(defun comment-beginning ()
"Find the beginning of the enclosing comment.

[Andreas Röhler]

syntax-ppss is reliable, while re-search-backward comment-start-skip might stop inside a string etc.

backward-line, end-of-line
if nt4 and nth8, goto char nth8

that's nearly all
as done consider limit of search, sure.

Watching with interest,

Andreas

[Leo Liu]

On 2013-05-16 00:13 +0800, Andreas Röhler wrote:
> syntax-ppss is reliable, while re-search-backward comment-start-skip
> might stop inside a string etc.
>
> backward-line, end-of-line
> if nt4 and nth8, goto char nth8
>
> that's nearly all
> as done consider limit of search, sure.
>
> Watching with interest,
>
> Andreas

I don't know what to make of this comment. Do you see a problem in the
patch?

Thanks,
Leo

[Andreas Röhler]

Yes, same thing as with beg-of-defun discussed elsewhere.

+ (while (and (not found) (re-search-backward comment-start-skip limit t))
+ (setq end (match-end 0))
+ (or (nth 8 (syntax-ppss)) (setq found t)))

This might find a comment-start inside a string.

Rely at (syntax-ppss)

if nt4 and nth8, goto char nth8

Cheers,

Andreas
> Thanks,
> Leo
>

[Stefan Monnier]

> So it is no longer fine for comment-search-backward to end up in a
> comment or string any more.

I'd like to know a bit more about "no longer": what has changed?

IIUC the change is that you want to use comment-search-backward in
a different circumstance, but I don't really understand what is
that circumstance.

> How about something like this?

It looks OK, though please only use syntax-ppss if comment-use-syntax is
set, because some modes still use comment commands in contexts where the
syntax tables do not reflect the intended comment syntax.


Stefan

[Leo Liu]

On 2013-05-16 21:28 +0800, Stefan Monnier wrote:
> I'd like to know a bit more about "no longer": what has changed?
>
> IIUC the change is that you want to use comment-search-backward in
> a different circumstance, but I don't really understand what is
> that circumstance.

In octave mode, insert the following line:

printf ("aaaa dddd dddd dddd dddd dddd dddd dddd dddd dddd dddd #i", abcd)

move point to the beginning of last word 'abcd' and type M-j.

Also people have been observing weird behaviour in auto-fill-mode or
fill-paragraph of comment chars insertion that seems come from nowhere.
For example, in early org mode versions (probably version 4 or so) there
have been constant reports of such behaviours.

Leo

[Stefan Monnier]

> In octave mode, insert the following line:
> printf ("aaaa dddd dddd dddd dddd dddd dddd dddd dddd dddd dddd #i", abcd)
> move point to the beginning of last word 'abcd' and type M-j.

Right, that makes sense.


Stefan

[Leo Liu]

On 2013-05-17 01:38 +0800, Stefan Monnier wrote:
> Right, that makes sense.
>
>
> Stefan

In that case I plan to install the patch as attached.

I have found that it is very easy for people who provide customised
comment-start-skip to introduce bugs. For example octave mode used to
have "\\s<+\\s-*" as comment start skip and was lucky to work in the
buggy comment-search-backward.

I wonder what to do here. Better documentation on comment-start-skip to
inform people that it is used both in re-search-forward/backward and
better be anchored properly?


diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index d55feaa3..db07e6a9 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -485,27 +485,26 @@ (defun comment-search-backward (&optional limit noerror)

+ (while (and (not found) (re-search-backward comment-start-skip limit t))
+ (setq end (match-end 0))

+ (unless (and comment-use-syntax (nth 8 (syntax-ppss)))
+ (setq found t)))

+ (if (not found)
+ (unless noerror (error "No comment"))
+ (beginning-of-line)
+ (let ((cs (comment-search-forward end t))
+ (pt (point)))
+ (if (not cs)
+ (progn (beginning-of-line)
+ (comment-search-backward limit noerror))
+ (while (progn (goto-char cs)
+ (comment-forward)
+ (and (< (point) end)
+ (setq cs (comment-search-forward end t))))
+ (setq pt (point)))
+ (goto-char pt)
+ cs)))))

(defun comment-beginning ()
"Find the beginning of the enclosing comment.

--
1.8.2

[Leo Liu]

On 2013-05-16 15:12 +0800, Andreas Röhler wrote:
> Yes, same thing as with beg-of-defun discussed elsewhere.
>

> + (while (and (not found) (re-search-backward comment-start-skip limit t))
> + (setq end (match-end 0))

> + (or (nth 8 (syntax-ppss)) (setq found t)))
>
> This might find a comment-start inside a string.
>
> Rely at (syntax-ppss)
>
> if nt4 and nth8, goto char nth8

There is possibility of ending up in a string. How about something along
these lines? Thanks. Leo

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index d55feaa3..79cdc393 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -485,27 +485,30 @@ (defun comment-search-backward (&optional limit noerror)

Moves point to inside the comment and returns the position of the
comment-starter. If no comment is found, moves point to LIMIT
and raises an error or returns nil if NOERROR is non-nil."
- ;; FIXME: If a comment-start appears inside a comment, we may erroneously
- ;; stop there. This can be rather bad in general, but since
- ;; comment-search-backward is only used to find the comment-column (in
- ;; comment-set-column) and to find the comment-start string (via
- ;; comment-beginning) in indent-new-comment-line, it should be harmless.
- (if (not (re-search-backward comment-start-skip limit t))
- (unless noerror (error "No comment"))
- (beginning-of-line)
- (let* ((end (match-end 0))
- (cs (comment-search-forward end t))
- (pt (point)))
- (if (not cs)
- (progn (beginning-of-line)
- (comment-search-backward limit noerror))
- (while (progn (goto-char cs)
- (comment-forward)
- (and (< (point) end)
- (setq cs (comment-search-forward end t))))
- (setq pt (point)))
- (goto-char pt)
- cs))))

+ (let (found end beg)

+ (while (and (not found) (re-search-backward comment-start-skip limit t))

+ (setq beg (or (match-end 1) (match-beginning 0))
+ end (match-end 0))
+ (when (or (not comment-use-syntax)
+ (and (not (nth 8 (syntax-ppss beg)))
+ (nth 4 (syntax-ppss end))))
+ (setq found t))
+ (goto-char beg))

[Andreas Röhler]

The succession of things doesn't look right yet.

if (eq comment-use-syntax nil)

re-search-backward based solution

which is very seldom. Grep shows 4 cases.

Otherwise syntax-ppss - , prog1 and nth 4 goto char nth 8 - or so.
Make sure wrong regexp isn't called then/no wrong matches.

Andreas

[Leo Liu]

On 2013-05-17 19:14 +0800, Andreas Röhler wrote:
> The succession of things doesn't look right yet.
>
> if (eq comment-use-syntax nil)
>
> re-search-backward based solution
>
> which is very seldom. Grep shows 4 cases.
>
> Otherwise syntax-ppss - , prog1 and nth 4 goto char nth 8 - or so.
> Make sure wrong regexp isn't called then/no wrong matches.
>
> Andreas

There is possible optimisation when 'end' is in a string. Other than
that do you have a case where my solution will fail?

Leo

[Andreas Röhler]

Am 17.05.2013 13:34, schrieb Leo Liu:

> On 2013-05-17 19:14 +0800, Andreas R�hler wrote:
>> The succession of things doesn't look right yet.
>>
>> if (eq comment-use-syntax nil)
>>
>> re-search-backward based solution
>>
>> which is very seldom. Grep shows 4 cases.
>>
>> Otherwise syntax-ppss - , prog1 and nth 4 goto char nth 8 - or so.
>> Make sure wrong regexp isn't called then/no wrong matches.
>>
>> Andreas
>
> There is possible optimisation when 'end' is in a string.

Don't understand. "end" can't be in a string, if syntax-ppss is used.

Other than
> that do you have a case where my solution will fail?

Not just fail.
However most if this code looks redundant, useless employing of re-search-..., which will slow down Emacs when called from a program.



>
> Leo
>

[Leo Liu]

On 2013-05-17 20:39 +0800, Andreas Röhler wrote:
> Don't understand. "end" can't be in a string, if syntax-ppss is used.
>
>
> Other than
>> that do you have a case where my solution will fail?
>
> Not just fail.
> However most if this code looks redundant, useless employing of
> re-search-..., which will slow down Emacs when called from a program.

Much as I would like to incorporate your suggestion I couldn't do so
without fully understand your messages. I think we have two options:

1. if you have copyright assignment to FSF, post your patch and I will
happily withdraw mine.

2. If not, leave my patch for Stefan to review.

Leo

[Stefan Monnier]

> + (while (and (not found) (re-search-backward comment-start-skip limit t))
> + (setq end (match-end 0))

> + (unless (and comment-use-syntax (nth 8 (syntax-ppss)))
> + (setq found t)))

BTW, a useful idiom for such loops is the "empty body" while loop.

(while (and (re-search-backward comment-start-skip limit t)
(progn
(setq end (match-end 0))
(and comment-use-syntax (nth 8 (syntax-ppss))))))

-- Stefan

[Stefan Monnier]

>> Yes, same thing as with beg-of-defun discussed elsewhere.
>>

>> + (while (and (not found) (re-search-backward comment-start-skip limit t))
>> + (setq end (match-end 0))

>> + (or (nth 8 (syntax-ppss)) (setq found t)))
>>
>> This might find a comment-start inside a string.
>>
>> Rely at (syntax-ppss)
>>
>> if nt4 and nth8, goto char nth8

> There is possibility of ending up in a string.

I don't understand when that can happen (when inside a string (nth
8 ppss) is also non-nil).


Stefan

[Leo Liu]

On 2013-05-17 21:28 +0800, Stefan Monnier wrote:
> I don't understand when that can happen (when inside a string (nth
> 8 ppss) is also non-nil).

I have

(defvar octave-comment-start-skip "\\(^\\|\\S<\\)\\(?:%!\\|\\s<+\\)\\s-*"
"Octave-specific `comment-start-skip' (which see).")

and this could find "#abc" as comment start where BEG is outside of
strings and comments but END is in a string.

Maybe this is due to setting octave-comment-start-skip incorrectly.

I looked at comment-normalize-vars and see it uses:

\\(\\(^\\|[^\\\n]\\)\\(\\\\\\\\\\)*\\)

as anchor but I don't understand fully.

Leo

[Andreas Röhler]

BTW what is the fastest way moving backward --searching comment-- when not inside a comment?

Thought at

(forward-line -1)
(end-of-line)
ppss-Check-for-Comment-again

maybe re-search-backward is as fast?

Andreas

[Stefan Monnier]

>> I don't understand when that can happen (when inside a string (nth
>> 8 ppss) is also non-nil).
> I have
> (defvar octave-comment-start-skip "\\(^\\|\\S<\\)\\(?:%!\\|\\s<+\\)\\s-*"
> "Octave-specific `comment-start-skip' (which see).")
> and this could find "#abc" as comment start where BEG is outside of
> strings and comments but END is in a string.

Ah, I see. That's easy to fix: just check the syntax-ppss state at the
position about which you care, i.e. (or (match-end 1) (match-beginning 0)),
rather than at the position at which re-search-backward puts you.

> Maybe this is due to setting octave-comment-start-skip incorrectly.

> I looked at comment-normalize-vars and see it uses:
> \\(\\(^\\|[^\\\n]\\)\\(\\\\\\\\\\)*\\)
> as anchor but I don't understand fully.

No, this is to try and avoid mis-recognizing \# (and \\\#, but not \\#)
as a comment starter when \ is an escape character.


Stefan

[Leo Liu]

On 2013-05-17 21:26 +0800, Stefan Monnier wrote:
> BTW, a useful idiom for such loops is the "empty body" while loop.
>
> (while (and (re-search-backward comment-start-skip limit t)
> (progn
> (setq end (match-end 0))
> (and comment-use-syntax (nth 8 (syntax-ppss))))))

Yes, I use this occasionally too. Since we need to know if a comment
start is found I don't use it here.

Leo

[Leo Liu]

On 2013-05-17 22:27 +0800, Andreas Röhler wrote:
> maybe re-search-backward is as fast?

Usually it is fast enough. I have experienced some slow cases in C-x [
but maybe that is due to really bad regexp.

Leo

[Leo Liu]

Fixed in trunk.

On 2013-05-17 23:52 +0800, Stefan Monnier wrote:
> Ah, I see. That's easy to fix: just check the syntax-ppss state at the
> position about which you care, i.e. (or (match-end 1) (match-beginning 0)),
> rather than at the position at which re-search-backward puts you.

Thanks a lot.

Leo

[Andreas Röhler]

Am 18.05.2013 00:52, schrieb Leo Liu:

> On 2013-05-17 22:27 +0800, Andreas R�hler wrote:
>> maybe re-search-backward is as fast?
>
> Usually it is fast enough. I have experienced some slow cases in C-x [
> but maybe that is due to really bad regexp.
>
> Leo
>

As this is a very basic routine, IMO every thinking to make it as fast as possible is well invested.
BTW it's your merit having addressed that item.

So just for the record maybe:

Why start searching backward with a re-? Why not search comment-start string, which should travel much faster?

Andreas

[Stefan Monnier]

> Why start searching backward with a re-? Why not search comment-start
> string, which should travel much faster?

Fast is good, but that would be simply wrong.


Stefan

[Andreas Röhler]

May you point me at a use-case, where comment-start-skip as regexp is needed?
I.e. a case where comment-start as string wouldn't do it.

Thanks,

Andreas

[Stefan Monnier]

> May you point me at a use-case, where comment-start-skip as regexp is needed?
> I.e. a case where comment-start as string wouldn't do it.

E.g. in C++, comment-start is typically "//" which won't find the
beginning of a /*...*/ comment.


Stefan

[Andreas Röhler]

Okay, see. Thanks.

With different ways to start a comment, the regexp seems inevitable.
However, emacs lisp and related should not need it.

Given it's not defined if not needed, the backward-search could speed up in this cases.


Andreas