On 03/01/2016 10:02 AM, Alan Mackenzie wrote: > Hello, Daniel and Lars. > > On Fri, Feb 26, 2016 at 04:48:13PM +1030, Lars Ingebrigtsen wrote: >> Daniel Colascione writes: > >>> // 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.