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

[Alan Mackenzie]

Hello, Daniel and Lars.

On Fri, Feb 26, 2016 at 04:48:13PM +1030, Lars Ingebrigtsen wrote:
> Daniel Colascione <dan.colascione@gmail.com> 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?

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

-- 
Alan Mackenzie (Nuremberg, Germany).