From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#7918: [PATCH] cc-mode: only the first clause of a for-loop should be checked for declarations Date: Tue, 1 Mar 2016 18:02:42 +0000 Message-ID: <20160301180242.GA6494@acm.fritz.box> References: <87k2lsuh96.fsf@gnus.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1456855298 12231 80.91.229.3 (1 Mar 2016 18:01:38 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 1 Mar 2016 18:01:38 +0000 (UTC) Cc: 7918@debbugs.gnu.org To: Daniel Colascione , Lars Ingebrigtsen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Mar 01 19:01:23 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aaobq-0002To-VP for geb-bug-gnu-emacs@m.gmane.org; Tue, 01 Mar 2016 19:01:23 +0100 Original-Received: from localhost ([::1]:51534 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaobq-0002In-BW for geb-bug-gnu-emacs@m.gmane.org; Tue, 01 Mar 2016 13:01:22 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:60910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaobi-0002Gq-ES for bug-gnu-emacs@gnu.org; Tue, 01 Mar 2016 13:01:20 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaobc-0005I4-DS for bug-gnu-emacs@gnu.org; Tue, 01 Mar 2016 13:01:14 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:59500) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaobW-0005Dd-A6; Tue, 01 Mar 2016 13:01:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84) (envelope-from ) id 1aaobW-0001G9-3Y; Tue, 01 Mar 2016 13:01:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org, bug-cc-mode@gnu.org Resent-Date: Tue, 01 Mar 2016 18:01:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 7918 X-GNU-PR-Package: emacs,cc-mode X-GNU-PR-Keywords: patch confirmed Original-Received: via spool by 7918-submit@debbugs.gnu.org id=B7918.14568552144773 (code B ref 7918); Tue, 01 Mar 2016 18:01:02 +0000 Original-Received: (at 7918) by debbugs.gnu.org; 1 Mar 2016 18:00:14 +0000 Original-Received: from localhost ([127.0.0.1]:56627 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aaoak-0001Eu-BM for submit@debbugs.gnu.org; Tue, 01 Mar 2016 13:00:14 -0500 Original-Received: from mail.muc.de ([193.149.48.3]:20316) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aaoai-0001El-Bi for 7918@debbugs.gnu.org; Tue, 01 Mar 2016 13:00:13 -0500 Original-Received: (qmail 73417 invoked by uid 3782); 1 Mar 2016 18:00:10 -0000 Original-Received: from acm.muc.de (p548A401F.dip0.t-ipconnect.de [84.138.64.31]) by colin.muc.de (tmda-ofmipd) with ESMTP; Tue, 01 Mar 2016 19:00:09 +0100 Original-Received: (qmail 6889 invoked by uid 1000); 1 Mar 2016 18:02:42 -0000 Content-Disposition: inline In-Reply-To: <87k2lsuh96.fsf@gnus.org> User-Agent: Mutt/1.5.24 (2015-08-30) X-Delivery-Agent: TMDA/1.1.12 (Macallan) X-Primary-Address: acm@muc.de X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:114266 Archived-At: 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? > -- > (domestic pets only, the antidote for overdose, milk.) > bloggy blog: http://lars.ingebrigtsen.no -- Alan Mackenzie (Nuremberg, Germany).