* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
@ 2013-09-03 1:26 Dmitry Gutov
2013-09-29 3:59 ` Dmitry Gutov
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Gutov @ 2013-09-03 1:26 UTC (permalink / raw)
To: 15251
In certain conditions, if I press SPC, and a string on the current line
contains text matching `comment-start-skip', the filling is performed,
and the newly created line starts with a comment.
Examples (point is at |, fill-column is 70):
ruby-mode:
aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a" a|
press SPC =>
aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a"
#a
js-mode:
aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a" aa|
press SPC =>
aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a"
//aa
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-03 1:26 bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string Dmitry Gutov
@ 2013-09-29 3:59 ` Dmitry Gutov
2013-09-29 13:12 ` Andreas Röhler
2013-09-30 18:26 ` Stefan Monnier
0 siblings, 2 replies; 17+ messages in thread
From: Dmitry Gutov @ 2013-09-29 3:59 UTC (permalink / raw)
To: 15251
Dmitry Gutov <dgutov@yandex.ru> writes:
> In certain conditions, if I press SPC, and a string on the current line
> contains text matching `comment-start-skip', the filling is performed,
> and the newly created line starts with a comment.
>
> Examples (point is at |, fill-column is 70):
>
> ruby-mode:
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a" a|
>
> press SPC =>
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a"
> #a
>
> js-mode:
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a" aa|
>
> press SPC =>
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a"
> //aa
I've tracked this to misbehaving `comment-beginning'. Since we start
outside of string, and the comment char(s) are inside the string on the
same line, the check comparing the face at point to
`font-lock-string-face' doesn't work.
And in ruby-mode's case, since interpolations use a different face, even
putting a similar guard to check at `cs' won't help.
As I see it, we either have to use `syntax-ppss' here, or trust
font-lock. The latter means not skipping other checks even if
`comment-end-skip' is defined and doesn't match at point, and also
only trusting "let's assume ... if we're on the same line" when
font-lock-mode is disabled.
How does this change look? Seems to work fine.
(As an aside, I'm having hard time understanding which search is
supposed to have set the match data used by `(match-end 0)' there. Is it
`comment-search-backward'? Or `(looking-at comment-end-skip)'?)
=== modified file 'lisp/newcomment.el'
--- lisp/newcomment.el 2013-06-18 17:57:56 +0000
+++ lisp/newcomment.el 2013-09-29 03:41:18 +0000
@@ -526,12 +526,13 @@
(and
;; For modes where comment-start and comment-end are the same,
;; the search above may have found a `ce' rather than a `cs'.
- (or (if comment-end-skip (not (looking-at comment-end-skip)))
- ;; Maybe font-lock knows that it's a `cs'?
+ (if comment-end-skip (not (looking-at comment-end-skip)))
+ (or ;; Maybe font-lock knows that it's a `cs'?
(eq (get-text-property (match-end 0) 'face)
'font-lock-comment-face)
- (unless (eq (get-text-property (point) 'face)
- 'font-lock-comment-face)
+ (unless (or (eq (get-text-property (point) 'face)
+ 'font-lock-comment-face)
+ font-lock-mode)
;; Let's assume it's a `cs' if we're on the same line.
(>= (line-end-position) pt)))
;; Make sure that PT is not past the end of the comment.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-29 3:59 ` Dmitry Gutov
@ 2013-09-29 13:12 ` Andreas Röhler
2013-09-29 15:16 ` Dmitry Gutov
2013-09-30 18:26 ` Stefan Monnier
1 sibling, 1 reply; 17+ messages in thread
From: Andreas Röhler @ 2013-09-29 13:12 UTC (permalink / raw)
To: 15251
Am 29.09.2013 05:59, schrieb Dmitry Gutov:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> In certain conditions, if I press SPC, and a string on the current line
>> contains text matching `comment-start-skip', the filling is performed,
>> and the newly created line starts with a comment.
>>
>> Examples (point is at |, fill-column is 70):
>>
>> ruby-mode:
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a" a|
>>
>> press SPC =>
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a"
>> #a
>>
>> js-mode:
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a" aa|
>>
>> press SPC =>
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a"
>> //aa
>
> I've tracked this to misbehaving `comment-beginning'. Since we start
> outside of string, and the comment char(s) are inside the string on the
> same line, the check comparing the face at point to
> `font-lock-string-face' doesn't work.
>
> And in ruby-mode's case, since interpolations use a different face, even
> putting a similar guard to check at `cs' won't help.
>
> As I see it, we either have to use `syntax-ppss' here, or trust
> font-lock. The latter means not skipping other checks even if
> `comment-end-skip' is defined and doesn't match at point, and also
> only trusting "let's assume ... if we're on the same line" when
> font-lock-mode is disabled.
>
> How does this change look? Seems to work fine.
>
> (As an aside, I'm having hard time understanding which search is
> supposed to have set the match data used by `(match-end 0)' there. Is it
> `comment-search-backward'? Or `(looking-at comment-end-skip)'?)
>
> === modified file 'lisp/newcomment.el'
> --- lisp/newcomment.el 2013-06-18 17:57:56 +0000
> +++ lisp/newcomment.el 2013-09-29 03:41:18 +0000
> @@ -526,12 +526,13 @@
> (and
> ;; For modes where comment-start and comment-end are the same,
> ;; the search above may have found a `ce' rather than a `cs'.
> - (or (if comment-end-skip (not (looking-at comment-end-skip)))
> - ;; Maybe font-lock knows that it's a `cs'?
> + (if comment-end-skip (not (looking-at comment-end-skip)))
> + (or ;; Maybe font-lock knows that it's a `cs'?
> (eq (get-text-property (match-end 0) 'face)
> 'font-lock-comment-face)
> - (unless (eq (get-text-property (point) 'face)
> - 'font-lock-comment-face)
> + (unless (or (eq (get-text-property (point) 'face)
> + 'font-lock-comment-face)
> + font-lock-mode)
> ;; Let's assume it's a `cs' if we're on the same line.
> (>= (line-end-position) pt)))
> ;; Make sure that PT is not past the end of the comment.
>
>
>
>
>
Reading the face for detecting basic things like comment is terrible.
What did the author smoke when writing this ;)
BTW from my experience jumping to (nth 8 (syntax-ppss)) works well.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-29 13:12 ` Andreas Röhler
@ 2013-09-29 15:16 ` Dmitry Gutov
2013-09-30 18:27 ` Stefan Monnier
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Gutov @ 2013-09-29 15:16 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 15251
Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
> Reading the face for detecting basic things like comment is terrible.
It sure is. It can be justified for very hot functions, but otherwise
`syntax-ppss' does a better job in (more or less) constant time.
But that code was written in 2001, a few months before `syntax-ppss' was
introduced.
> What did the author smoke when writing this ;)
Let's ask him. :)
Stefan, can we consider `syntax-ppss' fast enough at this point?
It will mean re-implementing `comment-search-backward' in terms of it,
and removing most of the code in `comment-beginning' definition.
AFAICT, both of these functions are not particularly hot, and are called
once or twice per user action, at most.
`comment-search-backward' can also be made to respect
`comment-use-syntax' (and use its current definition if that var's value
is nil), but if comments don't have valid entries in the syntax table,
font-lock won't recognize them (or will it?), and then the dance with
face properties is not useful either.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-29 3:59 ` Dmitry Gutov
2013-09-29 13:12 ` Andreas Röhler
@ 2013-09-30 18:26 ` Stefan Monnier
1 sibling, 0 replies; 17+ messages in thread
From: Stefan Monnier @ 2013-09-30 18:26 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 15251
> As I see it, we either have to use `syntax-ppss' here, or trust
> font-lock.
Relying on syntax-ppss would be better than relying font-lock faces.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-29 15:16 ` Dmitry Gutov
@ 2013-09-30 18:27 ` Stefan Monnier
2013-10-01 1:18 ` Dmitry Gutov
2013-10-02 7:22 ` Andreas Röhler
0 siblings, 2 replies; 17+ messages in thread
From: Stefan Monnier @ 2013-09-30 18:27 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 15251
> Stefan, can we consider `syntax-ppss' fast enough at this point?
If comment-use-syntax is t, yes.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-30 18:27 ` Stefan Monnier
@ 2013-10-01 1:18 ` Dmitry Gutov
2013-10-01 14:11 ` Stefan Monnier
2013-10-02 7:22 ` Andreas Röhler
1 sibling, 1 reply; 17+ messages in thread
From: Dmitry Gutov @ 2013-10-01 1:18 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 15251-done
Version: 24.4
On 30.09.2013 21:27, Stefan Monnier wrote:
>> Stefan, can we consider `syntax-ppss' fast enough at this point?
>
> If comment-use-syntax is t, yes.
Done, in 114486.
I kept the rest of `comment-beginning' definition, in case
non-syntax-table-using comments can still be font-locked in some modes.
Also, `comment-use-global-state' looks like a better var to base the
choice on, semantically, but it's not auto-detected, and it's only used
in `comment-search-forward'.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-01 1:18 ` Dmitry Gutov
@ 2013-10-01 14:11 ` Stefan Monnier
2013-10-01 15:40 ` Dmitry Gutov
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Monnier @ 2013-10-01 14:11 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 15251-done
> Also, `comment-use-global-state' looks like a better var to base the choice
Good point.
> on, semantically, but it's not auto-detected, and it's only used in
> comment-search-forward'.
We should probably rework the code to merge comment-use-syntax and
comment-use-global-state.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-01 14:11 ` Stefan Monnier
@ 2013-10-01 15:40 ` Dmitry Gutov
2013-10-02 0:32 ` Stefan Monnier
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Gutov @ 2013-10-01 15:40 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 15251-done
On 01.10.2013 17:11, Stefan Monnier wrote:
> We should probably rework the code to merge comment-use-syntax and
> comment-use-global-state.
I wonder what modes have the former variable set to t, but the latter to
nil, and which situation this combination handles.
Maybe enhance the check in `comment-normalize-vars' and set
`comment-use-syntax' to nil when the syntax table values are not good
enough for `comment-use-global-state'?
Then make `comment-use-global-state' an obsolete alias for
`comment-use-syntax'.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-01 15:40 ` Dmitry Gutov
@ 2013-10-02 0:32 ` Stefan Monnier
2013-10-02 0:50 ` Dmitry Gutov
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Monnier @ 2013-10-02 0:32 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 15251-done
>> We should probably rework the code to merge comment-use-syntax and
>> comment-use-global-state.
> I wonder what modes have the former variable set to t, but the latter to
> nil, and which situation this combination handles.
> Maybe enhance the check in `comment-normalize-vars' and set
> comment-use-syntax' to nil when the syntax table values are not good enough
> for `comment-use-global-state'?
> Then make `comment-use-global-state' an obsolete alias for
> comment-use-syntax'.
Maybe a better way to do it is:
- Change code that uses comment-use-global-state to use (and
comment-use-syntax comment-use-global-state) instead.
- Set comment-use-global-state to t by default.
- Major modes where comment-use-global-state is problematic (if those
exist) can then set comment-use-global-state to nil.
- Maybe make comment-use-global-state obsolete.
Context: the problem with syntax-ppss is not so much performance as
correctness, because syntax-ppss can get confused if you use several
syntax-tables in the same buffer (e.g. via font-lock-syntax-table), or
if you use narrowing.
As time goes on, more and more code relies on syntax-ppss so more and
more code gets rewritten to avoid font-lock-syntax-table (or at least
use it in "harmless" ways, e.g. only changing syntax from symbol to
word) and narrowing. In turn, this makes syntax-ppss more robust and
more attractive.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-02 0:32 ` Stefan Monnier
@ 2013-10-02 0:50 ` Dmitry Gutov
0 siblings, 0 replies; 17+ messages in thread
From: Dmitry Gutov @ 2013-10-02 0:50 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 15251-done
On 02.10.2013 03:32, Stefan Monnier wrote:
> - Change code that uses comment-use-global-state to use (and
> comment-use-syntax comment-use-global-state) instead.
This is already the case: `comment-search-forward' consults the value of
`comment-use-global-state' only when `comment-use-syntax' is truthy.
> - Maybe make comment-use-global-state obsolete.
>
> Context: the problem with syntax-ppss is not so much performance as
> correctness, because syntax-ppss can get confused if you use several
> syntax-tables in the same buffer (e.g. via font-lock-syntax-table), or
> if you use narrowing.
Yes, that seems hard to test for.
And if `syntax-ppss' can be borked in some modes, using its value in
`comment-beginning' like I did in the respective patch, also seems unsafe.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-09-30 18:27 ` Stefan Monnier
2013-10-01 1:18 ` Dmitry Gutov
@ 2013-10-02 7:22 ` Andreas Röhler
2013-10-02 11:19 ` Dmitry Gutov
1 sibling, 1 reply; 17+ messages in thread
From: Andreas Röhler @ 2013-10-02 7:22 UTC (permalink / raw)
To: Stefan Monnier, Dmitry Gutov; +Cc: 15251
Am 30.09.2013 20:27, schrieb Stefan Monnier:
>> Stefan, can we consider `syntax-ppss' fast enough at this point?
>
> If comment-use-syntax is t, yes.
>
>
> Stefan
>
In case not, an alternative to faces should be introduced still.
Setting of comment-face might fail for several reasons.
Comment-move should not rely on that.
What about searching for comment-start than?
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-02 7:22 ` Andreas Röhler
@ 2013-10-02 11:19 ` Dmitry Gutov
2013-10-02 12:24 ` Andreas Röhler
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Gutov @ 2013-10-02 11:19 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 15251
Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
> In case not, an alternative to faces should be introduced still.
> Setting of comment-face might fail for several reasons.
> Comment-move should not rely on that.
>
> What about searching for comment-start than?
`comment-beginning' calls `comment-search-backward', which searches for
`comment-start-skip'.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-02 11:19 ` Dmitry Gutov
@ 2013-10-02 12:24 ` Andreas Röhler
2013-10-02 12:25 ` Dmitry Gutov
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Röhler @ 2013-10-02 12:24 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 15251
Am 02.10.2013 13:19, schrieb Dmitry Gutov:
> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>> In case not, an alternative to faces should be introduced still.
>> Setting of comment-face might fail for several reasons.
>> Comment-move should not rely on that.
>>
>> What about searching for comment-start than?
>
> `comment-beginning' calls `comment-search-backward', which searches for
> `comment-start-skip'.
>
Okay, so it got rid of the faces-search?
Cheers
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-02 12:24 ` Andreas Röhler
@ 2013-10-02 12:25 ` Dmitry Gutov
2013-10-02 14:08 ` Andreas Röhler
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Gutov @ 2013-10-02 12:25 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 15251
On 02.10.2013 15:24, Andreas Röhler wrote:
> Okay, so it got rid of the faces-search?
Why don't you look at the source code?
`comment-beginning' looks at faces to make sure it's not inside a
string, or that the `comment-start-skip' it found doesn't end the
comment instead of starting it, in case comment-start and comment-end
delimiters are the same.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-02 12:25 ` Dmitry Gutov
@ 2013-10-02 14:08 ` Andreas Röhler
2013-10-04 2:09 ` Dmitry Gutov
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Röhler @ 2013-10-02 14:08 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 15251
Am 02.10.2013 14:25, schrieb Dmitry Gutov:
> On 02.10.2013 15:24, Andreas Röhler wrote:
>> Okay, so it got rid of the faces-search?
>
> Why don't you look at the source code?
>
> `comment-beginning' looks at faces to make sure it's not inside a string,
Doesn't make sense for me. In which way is a string supposed to bear string-face, if syntax doesn't exist?
That's shooting into the dark IMHO.
or that the `comment-start-skip' it found doesn't end the comment instead of starting it, in case
> comment-start and comment-end delimiters are the same.
>
Same here.
If syntax doesn't exist, only a parser may help.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string
2013-10-02 14:08 ` Andreas Röhler
@ 2013-10-04 2:09 ` Dmitry Gutov
0 siblings, 0 replies; 17+ messages in thread
From: Dmitry Gutov @ 2013-10-04 2:09 UTC (permalink / raw)
To: Andreas Röhler; +Cc: 15251
On 02.10.2013 17:08, Andreas Röhler wrote:
> Doesn't make sense for me. In which way is a string supposed to bear
> string-face, if syntax doesn't exist?
I'm just guessing, but syntax table may have syntax entries for string
delimiters, but not for comments (e.g. because their syntax is too weird
to be adequately expressed using existing syntax table classes), so
comments may be fontified via explicit font-lock-keywords entries.
Another option is, like Stefan mentioned, if font-lock-syntax-table is
defined, it's used for fontification, and it's very different from the
actual buffer syntax table.
> That's shooting into the dark IMHO.
That code is old. I'm pretty sure nobody would've written it if there
weren't any modes in the wild at the time that benefited from it.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-10-04 2:09 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03 1:26 bug#15251: 24.3.50; do-auto-fill "continues" comment from inside a string Dmitry Gutov
2013-09-29 3:59 ` Dmitry Gutov
2013-09-29 13:12 ` Andreas Röhler
2013-09-29 15:16 ` Dmitry Gutov
2013-09-30 18:27 ` Stefan Monnier
2013-10-01 1:18 ` Dmitry Gutov
2013-10-01 14:11 ` Stefan Monnier
2013-10-01 15:40 ` Dmitry Gutov
2013-10-02 0:32 ` Stefan Monnier
2013-10-02 0:50 ` Dmitry Gutov
2013-10-02 7:22 ` Andreas Röhler
2013-10-02 11:19 ` Dmitry Gutov
2013-10-02 12:24 ` Andreas Röhler
2013-10-02 12:25 ` Dmitry Gutov
2013-10-02 14:08 ` Andreas Röhler
2013-10-04 2:09 ` Dmitry Gutov
2013-09-30 18:26 ` Stefan Monnier
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).