From: Daniel Colascione <dancol@dancol.org>
To: Alan Mackenzie <acm@muc.de>,
Daniel Colascione <dan.colascione@gmail.com>,
Lars Ingebrigtsen <larsi@gnus.org>
Cc: 7918@debbugs.gnu.org
Subject: bug#7918: [PATCH] cc-mode: only the first clause of a for-loop should be checked for declarations
Date: Tue, 1 Mar 2016 10:05:47 -0800 [thread overview]
Message-ID: <56D5D9FB.2060605@dancol.org> (raw)
In-Reply-To: <20160301180242.GA6494@acm.fritz.box>
[-- Attachment #1.1: Type: text/plain, Size: 6620 bytes --]
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.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2016-03-01 18:05 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-01-26 6:36 bug#7918: [PATCH] cc-mode: only the first clause of a for-loop should be checked for declarations Daniel Colascione
2016-02-26 6:18 ` Lars Ingebrigtsen
2016-02-26 6:31 ` Daniel Colascione
2016-02-26 6:33 ` Daniel Colascione
2016-03-01 18:02 ` Alan Mackenzie
2016-03-01 18:05 ` Daniel Colascione [this message]
2016-04-01 16:18 ` Alan Mackenzie
2016-04-25 18:04 ` Alan Mackenzie
[not found] ` <handler.7918.D7918.146160769022226.notifdone@debbugs.gnu.org>
2017-06-29 1:06 ` npostavs
2017-07-03 19:09 ` Glenn Morris
2017-07-03 19:46 ` npostavs
2017-07-03 20:18 ` Alan Mackenzie
2017-07-05 15:55 ` Glenn Morris
2017-07-05 20:14 ` Alan Mackenzie
2017-07-06 1:39 ` Glenn Morris
2017-07-07 14:47 ` Alan Mackenzie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56D5D9FB.2060605@dancol.org \
--to=dancol@dancol.org \
--cc=7918@debbugs.gnu.org \
--cc=acm@muc.de \
--cc=dan.colascione@gmail.com \
--cc=larsi@gnus.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.