bug#7918: [PATCH] cc-mode: only the first clause of a for-loop should be checked for declarations

[Daniel Colascione]

+ ;; After a ";" in a for-block. A declaration can never
+ ;; begin after a `;' if the most enclosing paren is a
+ ;; `('.-declarators' handles the rest.

Hrm. That's not true in general. Consider Java's try-with-resources
construct:

try (Foo foo = new Foo(); Bar bar = new Bar()) { ... }

[Daniel Colascione]

Post by Daniel Colascione

+ ;; After a ";" in a for-block. A declaration can never
+ ;; begin after a `;' if the most enclosing paren is a
+ ;; `('.-declarators' handles the rest.

Hrm. That's not true in general. Consider Java's try-with-resources
try (Foo foo = new Foo(); Bar bar = new Bar()) { ... }

Gah, of course that's true in a `for' block specifically.

[Alan Mackenzie]

Hello, Daniel and Lars.

// This code has no variable declarations
void foo() {
for (; (DWORD) a * b ;)
;
for (; a * b ;)
;
}

I can confirm that the Emacs trunk still highlights the "a" in these
examples wrong, and that Daniel's patch seems to fix the issue.
However, I'm totally unfamiliar with the cc-mode code, so it would be
nice if somebody could look at it before it's applied.

OK. I haven't actually tried this patch out, but there are things in it
I find concerning.

=== modified file 'lisp/progmodes/cc-fonts.el'
--- lisp/progmodes/cc-fonts.el 2010-12-07 12:15:28 +0000
+++ lisp/progmodes/cc-fonts.el 2011-01-25 11:10:00 +0000
@@ -1080,7 +1080,8 @@
;; o - '<> if the arglist is of angle bracket type;
;; o - 'arglist if it's some other arglist;
;; o - nil, if not in an arglist at all. This includes the
- ;; parenthesised condition which follows "if", "while", etc.
+ ;; parenthesised condition which follows "if", "while", etc.,
+ ;; but not "for", which is 'arglist after `;'.

By what logic is `context' set to 'arglist in a "for" statement? The
main function of `context' is to inform `c-forward-decl-or-cast-1' of
the context in which it is being called.

context
;; The position of the next token after the closing paren of
;; the last detected cast.
@@ -1109,7 +1110,7 @@
;; `c-forward-decl-or-cast-1' and `c-forward-label' for
;; later fontification.
(c-record-type-identifiers t)
- label-type
+ label-type paren-state most-enclosing-brace
c-record-ref-identifiers
;; Make `c-forward-type' calls mark up template arglists if
;; it finds any. That's necessary so that we later will
@@ -1171,7 +1172,6 @@
'font-lock-function-name-face))))
(c-font-lock-function-postfix limit))
-
(setq start-pos (point))
(when
;; The result of the `if' condition below is true when we don't recognize a

The next hunk attempts to move the detection of a "for" statement here
from later in the function where it previously was. Why?

@@ -1189,7 +1189,31 @@
;; (e.g. "for (").
(let ((type (and (> match-pos (point-min))
(c-get-char-property (1- match-pos) 'c-type))))
- (cond ((not (memq (char-before match-pos) '(?\( ?, ?\[ ?<)))
+ (cond
+ (;; Try to not fontify the second and third clauses of
+ ;; `for' statements as declarations.
+ (and (or (eq (char-before match-pos) ?\;)
+ (save-excursion
+ ;; Catch things like for(; (DWORD)(int) x &
+ ;; y; ) without invoking the full might of
+ ;; c-beginning-of-statement-1.
+ (goto-char match-pos)
+ (while (eq (char-before) ?\))
+ (c-go-list-backward)
+ (c-backward-syntactic-ws))

Here we potentially have an infinite loop when there's an unbalanced ")"
in the code. It's critical to check the return from
`c-go-list-backward' here, too.

+ (eq (char-before) ?\;)))
+
+ (setq paren-state (c-parse-state))
+ (setq most-enclosing-brace
+ (c-most-enclosing-brace paren-state))
+ (eq (char-after most-enclosing-brace) ?\())

Rather than using `c-parse-state', this could be done more efficiently
with `c-up-list-backward'. There may be the possibility of an error
here if `c-most-enclosing-brace' returns nil, leading to (char-after
nil), but maybe that can't happen. It would certainly be a good idea to
check for it, though.

+
+ ;; After a ";" in a for-block. A declaration can never
+ ;; begin after a `;' if the most enclosing paren is a
+ ;; `('.

How do we know we're in a "for" block here? There is no `looking-at'
check with the pertinent regexp (c-paren-stmt-key).

+ (setq context 'arglist
+ c-restricted-<>-arglists t))

Again, why is `context' set to 'arglist here? What effect is this
intended to have on `c-forward-decl-or-cast-1'?

+ ((not (memq (char-before match-pos) '(?\( ?, ?\[ ?<)))
(setq context nil
c-restricted-<>-arglists nil))
;; A control flow expression
@@ -1252,7 +1276,7 @@
;; Are we at a declarator? Try to go back to the declaration
;; to check this. Note that `c-beginning-of-decl-1' is slow,
;; so we cache its result between calls.
- (let (paren-state bod-res encl-pos is-typedef)
+ (let (bod-res encl-pos is-typedef)
(goto-char start-pos)
(save-excursion
(unless (and decl-search-lim
@@ -1318,20 +1342,7 @@
;; Back up to the type to fontify the declarator(s).
(goto-char (car decl-or-cast))
- (let ((decl-list
- (if context
- ;; Should normally not fontify a list of
- ;; declarators inside an arglist, but the first
- ;; argument in the ';' separated list of a "for"
- ;; statement is an exception.
- (when (eq (char-before match-pos) ?\()
- (save-excursion
- (goto-char (1- match-pos))
- (c-backward-syntactic-ws)
- (and (c-simple-skip-symbol-backward)
- (looking-at c-paren-stmt-key))))
- t)))
-
+ (let ((decl-list (not context)))

Here the setting of decl-list is changed. Why? `decl-list''s purpose
is to instruct `c-font-lock-declarators' whether to fontify just one
declarator or a whole list of them. The existing logic is to fontify
all the declarators in a "for" block, whereas after the patch only the
first one would be fontified. Again, why?

;; Fix the `c-decl-id-start' or `c-decl-type-start' property
;; before the first declarator if it's a list.
;; `c-font-lock-declarators' handles the rest.

Question (for Daniel): has this patch been run through the CC Mode test
suite, yet?

--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no

[Daniel Colascione]

Post by Alan Mackenzie
Hello, Daniel and Lars.

// This code has no variable declarations
void foo() {
for (; (DWORD) a * b ;)
;
for (; a * b ;)
;
}

I can confirm that the Emacs trunk still highlights the "a" in these
examples wrong, and that Daniel's patch seems to fix the issue.
However, I'm totally unfamiliar with the cc-mode code, so it would be
nice if somebody could look at it before it's applied.

OK. I haven't actually tried this patch out, but there are things in it
I find concerning.

=== modified file 'lisp/progmodes/cc-fonts.el'
--- lisp/progmodes/cc-fonts.el 2010-12-07 12:15:28 +0000
+++ lisp/progmodes/cc-fonts.el 2011-01-25 11:10:00 +0000
@@ -1080,7 +1080,8 @@
;; o - '<> if the arglist is of angle bracket type;
;; o - 'arglist if it's some other arglist;
;; o - nil, if not in an arglist at all. This includes the
- ;; parenthesised condition which follows "if", "while", etc.
+ ;; parenthesised condition which follows "if", "while", etc.,
+ ;; but not "for", which is 'arglist after `;'.

By what logic is `context' set to 'arglist in a "for" statement? The
main function of `context' is to inform `c-forward-decl-or-cast-1' of
the context in which it is being called.

context
;; The position of the next token after the closing paren of
;; the last detected cast.
@@ -1109,7 +1110,7 @@
;; `c-forward-decl-or-cast-1' and `c-forward-label' for
;; later fontification.
(c-record-type-identifiers t)
- label-type
+ label-type paren-state most-enclosing-brace
c-record-ref-identifiers
;; Make `c-forward-type' calls mark up template arglists if
;; it finds any. That's necessary so that we later will
@@ -1171,7 +1172,6 @@
'font-lock-function-name-face))))
(c-font-lock-function-postfix limit))
-
(setq start-pos (point))
(when
;; The result of the `if' condition below is true when we don't recognize a

The next hunk attempts to move the detection of a "for" statement here
from later in the function where it previously was. Why?

@@ -1189,7 +1189,31 @@
;; (e.g. "for (").
(let ((type (and (> match-pos (point-min))
(c-get-char-property (1- match-pos) 'c-type))))
- (cond ((not (memq (char-before match-pos) '(?\( ?, ?\[ ?<)))
+ (cond
+ (;; Try to not fontify the second and third clauses of
+ ;; `for' statements as declarations.
+ (and (or (eq (char-before match-pos) ?\;)
+ (save-excursion
+ ;; Catch things like for(; (DWORD)(int) x &
+ ;; y; ) without invoking the full might of
+ ;; c-beginning-of-statement-1.
+ (goto-char match-pos)
+ (while (eq (char-before) ?\))
+ (c-go-list-backward)
+ (c-backward-syntactic-ws))

Here we potentially have an infinite loop when there's an unbalanced ")"
in the code. It's critical to check the return from
`c-go-list-backward' here, too.

+ (eq (char-before) ?\;)))
+
+ (setq paren-state (c-parse-state))
+ (setq most-enclosing-brace
+ (c-most-enclosing-brace paren-state))
+ (eq (char-after most-enclosing-brace) ?\())

Rather than using `c-parse-state', this could be done more efficiently
with `c-up-list-backward'. There may be the possibility of an error
here if `c-most-enclosing-brace' returns nil, leading to (char-after
nil), but maybe that can't happen. It would certainly be a good idea to
check for it, though.

+
+ ;; After a ";" in a for-block. A declaration can never
+ ;; begin after a `;' if the most enclosing paren is a
+ ;; `('.

How do we know we're in a "for" block here? There is no `looking-at'
check with the pertinent regexp (c-paren-stmt-key).

+ (setq context 'arglist
+ c-restricted-<>-arglists t))

Again, why is `context' set to 'arglist here? What effect is this
intended to have on `c-forward-decl-or-cast-1'?

+ ((not (memq (char-before match-pos) '(?\( ?, ?\[ ?<)))
(setq context nil
c-restricted-<>-arglists nil))
;; A control flow expression
@@ -1252,7 +1276,7 @@
;; Are we at a declarator? Try to go back to the declaration
;; to check this. Note that `c-beginning-of-decl-1' is slow,
;; so we cache its result between calls.
- (let (paren-state bod-res encl-pos is-typedef)
+ (let (bod-res encl-pos is-typedef)
(goto-char start-pos)
(save-excursion
(unless (and decl-search-lim
@@ -1318,20 +1342,7 @@
;; Back up to the type to fontify the declarator(s).
(goto-char (car decl-or-cast))
- (let ((decl-list
- (if context
- ;; Should normally not fontify a list of
- ;; declarators inside an arglist, but the first
- ;; argument in the ';' separated list of a "for"
- ;; statement is an exception.
- (when (eq (char-before match-pos) ?\()
- (save-excursion
- (goto-char (1- match-pos))
- (c-backward-syntactic-ws)
- (and (c-simple-skip-symbol-backward)
- (looking-at c-paren-stmt-key))))
- t)))
-
+ (let ((decl-list (not context)))

Here the setting of decl-list is changed. Why? `decl-list''s purpose
is to instruct `c-font-lock-declarators' whether to fontify just one
declarator or a whole list of them. The existing logic is to fontify
all the declarators in a "for" block, whereas after the patch only the
first one would be fontified. Again, why?

;; Fix the `c-decl-id-start' or `c-decl-type-start' property
;; before the first declarator if it's a list.
;; `c-font-lock-declarators' handles the rest.

Question (for Daniel): has this patch been run through the CC Mode test
suite, yet?

It has not. It's been years since I even thought about that code. If
you're up for it, I'd rather you supply a separate fix.

[Alan Mackenzie]

Hello, Daniel.

// This code has no variable declarations
void foo() {
for (; (DWORD) a * b ;)
;
for (; a * b ;)
;
}

It's been years since I even thought about that code. If you're up for
it, I'd rather you supply a separate fix.

OK, here goes:


diff -r f19a4ffb060b cc-fonts.el
--- a/cc-fonts.el Fri Apr 01 12:23:17 2016 +0000
+++ b/cc-fonts.el Fri Apr 01 16:10:57 2016 +0000
@@ -1206,8 +1206,20 @@
'font-lock-keyword-face)
(looking-at c-not-decl-init-keywords))
(and c-macro-with-semi-re
- (looking-at c-macro-with-semi-re))) ; 2008-11-04
- ;; Don't do anything more if we're looking at a keyword that
+ (looking-at c-macro-with-semi-re)) ; 2008-11-04
+ (save-excursion ; A construct after a ; in a `for' statement
+ ; can't be a declaration.
+ (and (c-go-up-list-backward)
+ (eq (char-after) ?\()
+ (progn (c-backward-syntactic-ws)
+ (c-simple-skip-symbol-backward))
+ (looking-at c-paren-stmt-key)
+ (progn (goto-char match-pos)
+ (while (and (eq (char-before) ?\))
+ (c-go-list-backward))
+ (c-backward-syntactic-ws))
+ (eq (char-before) ?\;)))))
+ ;; Don't do anything more if we're looking at something that
;; can't start a declaration.
t


Could you do the usual with this patch, please, then if everything's OK,
I can commit it to the emacs-25 branch. Thanks!

[Alan Mackenzie]

Bug fixed in master branch.

[Glenn Morris]

Post by Alan Mackenzie
Bug fixed in master branch.

I can confirm it's fixed in master (I wonder why the bug was reopened,
apparently automatically?)

Nothing gets reopened automatically. I agree that sometimes debbugs
doesn't report the details of control requests in an informative way.
In this case, the bug was reopened in May 2016 (by Alan, from internal
logs). You'd have to ask him why.

http://lists.gnu.org/archive/html/emacs-bug-tracker/2016-05/msg00126.html

[Alan Mackenzie]

Hello, Noam and Glenn.

Post by Glenn Morris

Post by Alan Mackenzie
Bug fixed in master branch.

I can confirm it's fixed in master (I wonder why the bug was reopened,
apparently automatically?)

Nothing gets reopened automatically. I agree that sometimes debbugs
doesn't report the details of control requests in an informative way.
In this case, the bug was reopened in May 2016 (by Alan, from internal
logs). You'd have to ask him why.

It was because the original "fix" for the bug slowed CC Mode's
fontification down by a factor of ~3.

Post by Glenn Morris
http://lists.gnu.org/archive/html/emacs-bug-tracker/2016-05/msg00126.html

Oh, I was looking at
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=7918;msg=32. So all those
"fake control message" things are just the original control request
getting lost somehow?
Ccing Alan, please reopen if I made a mistake by closing this.

This is strange. I sent an email attempting to close the bug for a
second time on 2016-05-16. This email was actually received and
acknowledged by debbugs.gnu.org. But apparently the bug didn't get
closed. Maybe sending mail to 7918-***@debbugs.gnu.org only works the
first time around.

Anyway, it's fine for the bug finally to be marked closed. Thanks.

[Glenn Morris]

Post by Alan Mackenzie

Oh, I was looking at
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=7918;msg=32. So all those
"fake control message" things are just the original control request
getting lost somehow?

Yes, I think it's basically a debbugs bug.

Post by Alan Mackenzie
time around.

Nope. There's no record of that second mail anywhere AFAICS.

[Alan Mackenzie]

Hello, Glenn.

Post by Glenn Morris

Post by Alan Mackenzie

Oh, I was looking at
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=7918;msg=32. So all those
"fake control message" things are just the original control request
getting lost somehow?

Yes, I think it's basically a debbugs bug.

Post by Alan Mackenzie
time around.

Nope. There's no record of that second mail anywhere AFAICS.

Oh, but there is. In my qmail log for 2016-05-16 appears the following
entry:

2016-05-16 11:38:16.645 +0000 new msg 4072519
2016-05-16 11:38:16.645 +0000 info msg 4072519: bytes 2479 from <***@acm.muc.de> qp 19336 uid 1000
2016-05-16 11:38:16.719 +0000 starting delivery 73: msg 4072519 to remote 7918-***@debbugs.gnu.org
2016-05-16 11:38:16.719 +0000 status: local 0/10 remote 1/20
2016-05-16 11:38:17.974 +0000 delivery 73: success: 193.149.48.3_accepted_message./Remote_host_said:_250_Ok/
2016-05-16 11:38:17.975 +0000 status: local 0/10 remote 0/20
2016-05-16 11:38:17.975 +0000 end msg 4072519

This relates to my second message to 7918-***@debbugs.gnu.org, which was
clearly received by debbugs, just not acted upon.

Whether it's worth following up in any way is the question. I'd be
inclined just to let the matter drop, unless the same thing happens again
at some stage.

[Glenn Morris]

Post by Alan Mackenzie
2016-05-16 11:38:16.645 +0000 new msg 4072519
2016-05-16 11:38:16.645 +0000 info msg 4072519: bytes 2479 from
2016-05-16 11:38:16.719 +0000 starting delivery 73: msg 4072519 to
2016-05-16 11:38:16.719 +0000 status: local 0/10 remote 1/20
193.149.48.3_accepted_message./Remote_host_said:_250_Ok/

193.149.48.3 = mail.muc.de

Post by Alan Mackenzie
2016-05-16 11:38:17.975 +0000 status: local 0/10 remote 0/20
2016-05-16 11:38:17.975 +0000 end msg 4072519
clearly received by debbugs, just not acted upon.

AFAICS there's nothing in the above that shows the mail was received by
debbugs. It was received by mail.muc.de, but then who knows...
There's no sign of it in any debbugs logs.

[Alan Mackenzie]

Hello, Glenn.

Post by Glenn Morris

Post by Alan Mackenzie
2016-05-16 11:38:16.645 +0000 new msg 4072519
2016-05-16 11:38:16.645 +0000 info msg 4072519: bytes 2479 from
2016-05-16 11:38:16.719 +0000 starting delivery 73: msg 4072519 to
2016-05-16 11:38:16.719 +0000 status: local 0/10 remote 1/20
193.149.48.3_accepted_message./Remote_host_said:_250_Ok/

193.149.48.3 = mail.muc.de

Post by Alan Mackenzie
2016-05-16 11:38:17.975 +0000 status: local 0/10 remote 0/20
2016-05-16 11:38:17.975 +0000 end msg 4072519
clearly received by debbugs, just not acted upon.

AFAICS there's nothing in the above that shows the mail was received by
debbugs. It was received by mail.muc.de, but then who knows...
There's no sign of it in any debbugs logs.

Yes, you're right. Sorry about my misunderstanding. All we can say is
that my email to 7918-done got lost somewhere between mail.muc.de and
debbugs.gnu.org.