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

[Daniel Colascione]

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 <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?

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.

signature.asc
Description: OpenPGP digital signature