* bug#14303: 24.3; Bug in comment-search-backward
@ 2013-04-29 13:27 Leo Liu
2013-05-15 11:33 ` Leo Liu
0 siblings, 1 reply; 27+ messages in thread
From: Leo Liu @ 2013-04-29 13:27 UTC (permalink / raw)
To: 14303
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.
Due to this bug octave-mode users are seeing mysterious comment char
such as % or # inserted by fill-paragraph or auto-fill-mode.
Leo
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-04-29 13:27 bug#14303: 24.3; Bug in comment-search-backward Leo Liu
@ 2013-05-15 11:33 ` Leo Liu
2013-05-15 16:13 ` Andreas Röhler
2013-05-16 13:28 ` Stefan Monnier
0 siblings, 2 replies; 27+ messages in thread
From: Leo Liu @ 2013-05-15 11:33 UTC (permalink / raw)
To: 14303
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.
^ permalink raw reply related [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-15 11:33 ` Leo Liu
@ 2013-05-15 16:13 ` Andreas Röhler
2013-05-16 4:02 ` Leo Liu
2013-05-16 13:28 ` Stefan Monnier
1 sibling, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-15 16:13 UTC (permalink / raw)
To: 14303
Am 15.05.2013 13:33, schrieb 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.
>
>
>
>
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-15 16:13 ` Andreas Röhler
@ 2013-05-16 4:02 ` Leo Liu
2013-05-16 7:12 ` Andreas Röhler
0 siblings, 1 reply; 27+ messages in thread
From: Leo Liu @ 2013-05-16 4:02 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-16 4:02 ` Leo Liu
@ 2013-05-16 7:12 ` Andreas Röhler
2013-05-17 10:47 ` Leo Liu
0 siblings, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-16 7:12 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
Am 16.05.2013 06:02, schrieb 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?
>
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-16 7:12 ` Andreas Röhler
@ 2013-05-17 10:47 ` Leo Liu
2013-05-17 11:14 ` Andreas Röhler
2013-05-17 13:28 ` Stefan Monnier
0 siblings, 2 replies; 27+ messages in thread
From: Leo Liu @ 2013-05-17 10:47 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303
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))
+ (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.
^ permalink raw reply related [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 10:47 ` Leo Liu
@ 2013-05-17 11:14 ` Andreas Röhler
2013-05-17 11:34 ` Leo Liu
2013-05-17 13:28 ` Stefan Monnier
1 sibling, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-17 11:14 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
Am 17.05.2013 12:47, schrieb 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))
> + (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.
>
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 11:14 ` Andreas Röhler
@ 2013-05-17 11:34 ` Leo Liu
2013-05-17 12:39 ` Andreas Röhler
0 siblings, 1 reply; 27+ messages in thread
From: Leo Liu @ 2013-05-17 11:34 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 11:34 ` Leo Liu
@ 2013-05-17 12:39 ` Andreas Röhler
2013-05-17 13:18 ` Leo Liu
0 siblings, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-17 12:39 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 12:39 ` Andreas Röhler
@ 2013-05-17 13:18 ` Leo Liu
0 siblings, 0 replies; 27+ messages in thread
From: Leo Liu @ 2013-05-17 13:18 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 10:47 ` Leo Liu
2013-05-17 11:14 ` Andreas Röhler
@ 2013-05-17 13:28 ` Stefan Monnier
2013-05-17 13:37 ` Leo Liu
1 sibling, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-17 13:28 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 13:28 ` Stefan Monnier
@ 2013-05-17 13:37 ` Leo Liu
2013-05-17 14:27 ` Andreas Röhler
2013-05-17 15:52 ` Stefan Monnier
0 siblings, 2 replies; 27+ messages in thread
From: Leo Liu @ 2013-05-17 13:37 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 13:37 ` Leo Liu
@ 2013-05-17 14:27 ` Andreas Röhler
2013-05-17 22:52 ` Leo Liu
2013-05-17 15:52 ` Stefan Monnier
1 sibling, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-17 14:27 UTC (permalink / raw)
To: 14303; +Cc: Leo
Am 17.05.2013 15:37, schrieb 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
>
>
>
>
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 14:27 ` Andreas Röhler
@ 2013-05-17 22:52 ` Leo Liu
2013-05-18 5:23 ` Andreas Röhler
0 siblings, 1 reply; 27+ messages in thread
From: Leo Liu @ 2013-05-17 22:52 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 22:52 ` Leo Liu
@ 2013-05-18 5:23 ` Andreas Röhler
2013-05-21 1:55 ` Stefan Monnier
0 siblings, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-18 5:23 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-18 5:23 ` Andreas Röhler
@ 2013-05-21 1:55 ` Stefan Monnier
2013-05-22 10:58 ` Andreas Röhler
0 siblings, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-21 1:55 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303, Leo Liu
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-21 1:55 ` Stefan Monnier
@ 2013-05-22 10:58 ` Andreas Röhler
2013-05-22 15:58 ` Stefan Monnier
0 siblings, 1 reply; 27+ messages in thread
From: Andreas Röhler @ 2013-05-22 10:58 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14303, Leo Liu
Am 21.05.2013 03:55, schrieb 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
>
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-22 10:58 ` Andreas Röhler
@ 2013-05-22 15:58 ` Stefan Monnier
2013-05-22 16:53 ` Andreas Röhler
0 siblings, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-22 15:58 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 14303, Leo Liu
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-22 15:58 ` Stefan Monnier
@ 2013-05-22 16:53 ` Andreas Röhler
0 siblings, 0 replies; 27+ messages in thread
From: Andreas Röhler @ 2013-05-22 16:53 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14303, Leo Liu
Am 22.05.2013 17:58, schrieb 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
>
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 13:37 ` Leo Liu
2013-05-17 14:27 ` Andreas Röhler
@ 2013-05-17 15:52 ` Stefan Monnier
2013-05-17 22:54 ` Leo Liu
1 sibling, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-17 15:52 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 15:52 ` Stefan Monnier
@ 2013-05-17 22:54 ` Leo Liu
0 siblings, 0 replies; 27+ messages in thread
From: Leo Liu @ 2013-05-17 22:54 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14303-done
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-15 11:33 ` Leo Liu
2013-05-15 16:13 ` Andreas Röhler
@ 2013-05-16 13:28 ` Stefan Monnier
2013-05-16 15:50 ` Leo Liu
1 sibling, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-16 13:28 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-16 13:28 ` Stefan Monnier
@ 2013-05-16 15:50 ` Leo Liu
2013-05-16 17:38 ` Stefan Monnier
0 siblings, 1 reply; 27+ messages in thread
From: Leo Liu @ 2013-05-16 15:50 UTC (permalink / raw)
To: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-16 15:50 ` Leo Liu
@ 2013-05-16 17:38 ` Stefan Monnier
2013-05-17 0:35 ` Leo Liu
0 siblings, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-16 17:38 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-16 17:38 ` Stefan Monnier
@ 2013-05-17 0:35 ` Leo Liu
2013-05-17 13:26 ` Stefan Monnier
0 siblings, 1 reply; 27+ messages in thread
From: Leo Liu @ 2013-05-17 0:35 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14303
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)
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))
+ (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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 0:35 ` Leo Liu
@ 2013-05-17 13:26 ` Stefan Monnier
2013-05-17 22:51 ` Leo Liu
0 siblings, 1 reply; 27+ messages in thread
From: Stefan Monnier @ 2013-05-17 13:26 UTC (permalink / raw)
To: Leo Liu; +Cc: 14303
> + (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
^ permalink raw reply [flat|nested] 27+ messages in thread* bug#14303: 24.3; Bug in comment-search-backward
2013-05-17 13:26 ` Stefan Monnier
@ 2013-05-17 22:51 ` Leo Liu
0 siblings, 0 replies; 27+ messages in thread
From: Leo Liu @ 2013-05-17 22:51 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14303
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
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2013-05-22 16:53 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-29 13:27 bug#14303: 24.3; Bug in comment-search-backward Leo Liu
2013-05-15 11:33 ` Leo Liu
2013-05-15 16:13 ` Andreas Röhler
2013-05-16 4:02 ` Leo Liu
2013-05-16 7:12 ` Andreas Röhler
2013-05-17 10:47 ` Leo Liu
2013-05-17 11:14 ` Andreas Röhler
2013-05-17 11:34 ` Leo Liu
2013-05-17 12:39 ` Andreas Röhler
2013-05-17 13:18 ` Leo Liu
2013-05-17 13:28 ` Stefan Monnier
2013-05-17 13:37 ` Leo Liu
2013-05-17 14:27 ` Andreas Röhler
2013-05-17 22:52 ` Leo Liu
2013-05-18 5:23 ` Andreas Röhler
2013-05-21 1:55 ` Stefan Monnier
2013-05-22 10:58 ` Andreas Röhler
2013-05-22 15:58 ` Stefan Monnier
2013-05-22 16:53 ` Andreas Röhler
2013-05-17 15:52 ` Stefan Monnier
2013-05-17 22:54 ` Leo Liu
2013-05-16 13:28 ` Stefan Monnier
2013-05-16 15:50 ` Leo Liu
2013-05-16 17:38 ` Stefan Monnier
2013-05-17 0:35 ` Leo Liu
2013-05-17 13:26 ` Stefan Monnier
2013-05-17 22:51 ` Leo Liu
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).