From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Nix Newsgroups: gmane.emacs.devel Subject: [RFC PATCH] setting indentation styles via `c-file-style' fails to actually change indentation Date: Wed, 18 May 2011 00:21:10 +0100 Message-ID: <87pqngewwp.fsf@spindle.srvr.nix> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: dough.gmane.org 1305674490 4318 80.91.229.12 (17 May 2011 23:21:30 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 17 May 2011 23:21:30 +0000 (UTC) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed May 18 01:21:26 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1QMTZu-0001Ok-8U for ged-emacs-devel@m.gmane.org; Wed, 18 May 2011 01:21:26 +0200 Original-Received: from localhost ([::1]:54420 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMTZt-0003x0-4k for ged-emacs-devel@m.gmane.org; Tue, 17 May 2011 19:21:25 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:41600) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMTZp-0003wr-DR for emacs-devel@gnu.org; Tue, 17 May 2011 19:21:22 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QMTZn-0008JI-Hb for emacs-devel@gnu.org; Tue, 17 May 2011 19:21:21 -0400 Original-Received: from icebox.esperi.org.uk ([81.187.191.129]:41552 helo=mail.esperi.org.uk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMTZm-0008Ii-Rl for emacs-devel@gnu.org; Tue, 17 May 2011 19:21:19 -0400 Original-Received: from esperi.org.uk (nix@spindle.srvr.nix [192.168.14.15]) by mail.esperi.org.uk (8.14.4/8.14.3) with ESMTP id p4HNLAsR019218 for ; Wed, 18 May 2011 00:21:10 +0100 Original-Received: (from nix@localhost) by esperi.org.uk (8.14.4/8.12.11/Submit) id p4HNLA5l016920; Wed, 18 May 2011 00:21:10 +0100 Emacs: impress your (remaining) friends and neighbors. User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) X-DCC-INFN-TO-Metrics: spindle 1233; Body=1 Fuz1=1 Fuz2=1 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 81.187.191.129 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:139472 Archived-At: Here's an experiment for you. Visit a .c file in c-mode, with 'c-old-style-variable-behavior' set to its default value of 'nil'. Set the style to something other than 'gnu' with 'c-set-style' (make it something with a different 'c-basic-offset'). Look at 'c-basic-offset'. It's changed. Great! Now. Add that same c-file-style as a file-local or directory-local variable to that other style. Reopen it. 'c-basic-offset' hasn't changed from the 'gnu' default, but 'c-indentation-style' proclaims that the style has in fact been set. In fact, 'c-basic-offset' is permanently stuck at the value specified for 'gnu', no matter what style you asked for. And this is true for every single variable that is set by the 'gnu' style, and for the pieces of 'c-offsets-alist' as well. What's going on? The problem is in 'c-set-style-1', where variables other than 'c-offsets-alist' and 'c-special-indent-hook' are treated specially: ;; all other variables (t (when (or (not dont-override) (not (memq attr c-style-variables)) (eq (if (eq dont-override t) (symbol-value attr) (default-value attr)) 'set-from-style)) (set attr val) The 'set-from-style' stuff is implementing the DONT-OVERRIDE parameter to c-set-style, which is annoyingly described only in terms of what it does, not what it's intended for: ,---- | If DONT-OVERRIDE is neither nil nor t, style variables whose default values | have been set (more precisely, whose default values are not the symbol | `set-from-style') will not be changed. This avoids overriding global settings | done in ~/.emacs. It is useful to call c-set-style from a mode hook in this | way. | | If DONT-OVERRIDE is t, style variables that already have values (i.e., whose | values are not the symbol `set-from-style') will not be overridden. CC Mode | calls c-set-style internally in this way whilst initializing a buffer; if | cc-set-style is called like this from anywhere else, it will usually behave as | a null operation. `---- DONT-OVERRIDE is clearly doing what it is specified to do. However, since 'c-set-style' may be called more than once when initializing a buffer, the net effect of these semantics is to set variables which are defined in the 'c-default-style' as well as in the 'c-file-style' to their values in the 'c-default-style', which is surely wrong: the 'c-default-style' is a global fallback, and should not take precedence over per-file or per-directory indentation styles. We could fix all of this by not installing the default style at all if 'c-file-style' is set (whether in file-local or dir-local variables, we do not care)... except that if c-file-style is set to a value which is filtered out by the 'hack-local-variables-filter', we *do* want to install the default style. And of course 'hack-local-variables' is not always called (e.g. it is not if you just run 'c-mode') and in that situation you presumably *do* want to set the style to the default. So this seems excessively complex and fragile: we don't want code inside c-mode to depend on the precise circumstances in which files.el chooses to call 'hack-local-variables', nor do we want to end up with situations in which c-mode starts up without setting an indentation style at all. This tangle is most easily escaped by adding an extra valid value to c-set-style's DONT-OVERRIDE, 'defaulting'. This is passed down when defaulting the style if and only if 'c-old-style-variable-behavior' and 'c-indentation-style' are both nil, indicating that style variables override the defaults and that we are not doing a subsequent style switch in global style mode. 'c-set-style' (or, rather, 'c-set-style-1') responds to this new value by adding the variable's name to a new list 'c-defaulted-style-variables' (buffer-local iff 'c-style-variables-are-local-p' is t); other calls to 'c-set-style' will override this variable's value both if its value is 'set-from-style' (as now) *and* if it is on the 'c-defaulted-style-variables' list. (There is another, similar variable 'c-defaulted-style-offsets' to track defaulted values within 'c-offsets-alist', because if the default style has set a number of values in this list, we want to be able to reset some of them in the file-local style, some in the dir-local style, and some in any styles it may inherit from, but without permitting the inherited style to override offset values in the the more-derived style. Currently, this machinery is so broken that even ordinary inherited styles aren't working: if you derive 'otbs' from 'bsd', the settings in 'bsd' override the settings in 'otbs' rather than the other way around.) The net effect of all this is that variables defined only in the default style and not in the c-file-style or manually-applied style stick around: other variables are immediately overwritten by their c-file-style or manually-applied values. It seems to me that this is precisely what is normally wanted. (The only situation in which this might go wrong is if you enter a buffer in the default style, change a style variable manually, then set another style and pass in t for DONT-OVERRIDE. In this situation, the 'c-set-style' will take precedence, while in the current system the style variable would. This is probably not a significant change: you're not supposed to pass in a DONT-OVERRIDE of t yourself anyway.) This is very confusing stuff (and the above is much too long for a log message). The result seems to work, but could do with review: it may well be that there is a way to do this that does not require two new lists whose sole purpose is to filter the application of style variables. (I thought that some clever reordering so that default styles were somehow applied after any non-default ones might have worked until I discovered that inherited styles were broken too.) Certainly the state of play before this change works worse, but that's about as confident as I'm willing to be. 2011-05-18 Nix * progmodes/cc-mode.el (c-basic-common-init): Pass in 'defaulting to `c-set-style' when setting up the `c-default-style'. * progmodes/cc-styles.el (c-style-defaulted-variables): New. * progmodes/cc-styles.el (c-style-defaulted-offsets): New. * progmodes/cc-styles.el (c-set-style-1): Use it to permit otherwise-filtered offsets and variables to be set, not * progmodes/cc-styles.el (c-set-style): * progmodes/cc-styles.el (c-make-styles-buffer-local): * progmodes/cc-styles.el (cc-styles): === modified file 'lisp/progmodes/cc-mode.el' Index: lisp/progmodes/cc-mode.el =================================================================== --- lisp/progmodes/cc-mode.el.orig 2011-05-17 16:01:50.000000000 +0100 +++ lisp/progmodes/cc-mode.el 2011-05-17 16:07:41.870198468 +0100 @@ -565,11 +565,12 @@ ;; the whole situation with nonlocal style variables is a bit ;; awkward. It's at least the most compatible way with the old ;; style init procedure.) - (c-set-style style (not (or c-old-style-variable-behavior - (and (not c-style-variables-are-local-p) - c-indentation-style - (not (string-equal c-indentation-style - style))))))) + (c-set-style style (if (not (or c-old-style-variable-behavior + (and (not c-style-variables-are-local-p) + c-indentation-style + (not (string-equal c-indentation-style + style))))) + 'defaulting))) (c-setup-paragraph-variables) ;; we have to do something special for c-offsets-alist so that the Index: lisp/progmodes/cc-styles.el =================================================================== --- lisp/progmodes/cc-styles.el.orig 2011-05-17 16:01:50.000000000 +0100 +++ lisp/progmodes/cc-styles.el 2011-05-17 23:57:52.719539336 +0100 @@ -271,6 +271,21 @@ modify existing styles -- you should create a new style that inherits the existing style).") +(defvar c-style-defaulted-variables '() +"A list of variables that have been defaulted by the application of a style. + +This list contains only those variables that have been set by the +`c-default-style' but not subsequently overridden by a `c-set-style' call. + +`c-offsets-alist' is not represented in this list at all, but rather in +`c-style-defaulted-offsets'.") + +(defvar c-style-defaulted-offsets '() +"A list of offsets that have been defaulted by the application of a style. + +This list contains only those offsets that have been set by the +`c-default-style' but not subsequently overridden by a `c-set-style' call.") + ;; Functions that manipulate styles (defun c-set-style-1 (conscell dont-override) @@ -284,11 +299,29 @@ c-offsets-alist) (dont-override (default-value 'c-offsets-alist))))) + ;; If values in the offsets list have been defaulted, don't screen them + ;; out, even under dont-override. + (if (and c-style-defaulted-offsets offsets) + (let (accum) + (mapc (lambda (offset) + (unless (memq (car offset) c-style-defaulted-offsets) + (setq accum (cons offset accum)))) + offsets) + (setq offsets accum))) + ;; Set everything that isn't in the offsets list; remove such things from + ;; c-style-defaulted-offsets unless this is a defaulting run, in which case + ;; add them instead (mapcar (lambda (langentry) (let ((langelem (car langentry)) (offset (cdr langentry))) (unless (assq langelem offsets) - (c-set-offset langelem offset)))) + (c-set-offset langelem offset) + (when (and (not (eq dont-override 'defaulting)) + (memq langelem c-style-defaulted-offsets)) + (setq c-style-defaulted-offsets (delq langelem c-style-defaulted-offsets)))) + (when (and (eq dont-override 'defaulting) + (not (memq langelem c-style-defaulted-offsets))) + (setq c-style-defaulted-offsets (cons langelem c-style-defaulted-offsets))))) val))) ;; second special variable ((eq attr 'c-special-indent-hook) @@ -296,6 +329,8 @@ ;; hooks? (unless (cond ((eq dont-override t) c-special-indent-hook) + ((memq 'c-special-indent-hook c-style-defaulted-variables) + nil) (dont-override (default-value 'c-special-indent-hook))) (if (listp val) @@ -306,6 +341,7 @@ ;; all other variables (t (when (or (not dont-override) (not (memq attr c-style-variables)) + (memq attr c-style-defaulted-variables) (eq (if (eq dont-override t) (symbol-value attr) (default-value attr)) @@ -314,7 +350,15 @@ ;; Must update a number of other variables if ;; c-comment-prefix-regexp is set. (if (eq attr 'c-comment-prefix-regexp) - (c-setup-paragraph-variables))))))) + (c-setup-paragraph-variables))))) + + ;; If this is a run to set the default style, note that this variable can + ;; now be freely overwridden by later calls to this function. Otherwise, + ;; take it out again: it's been overridden, if it was there. + (if (and (eq dont-override 'defaulting) + (not (eq attr 'c-offsets-alist))) ; this is recorded elsewhere + (add-to-list 'c-style-defaulted-variables attr) + (setq c-style-defaulted-variables (remq attr c-style-defaulted-variables))))) (defun c-get-style-variables (style basestyles) ;; Return all variables in a style by resolving inheritances. @@ -351,7 +395,7 @@ If DONT-OVERRIDE is neither nil nor t, style variables whose default values have been set (more precisely, whose default values are not the symbol -`set-from-style') will not be changed. This avoids overriding global settings +`set-from-style') will not be changed. This avoids overriding global settings done in ~/.emacs. It is useful to call c-set-style from a mode hook in this way. @@ -359,7 +403,12 @@ values are not the symbol `set-from-style') will not be overridden. CC Mode calls c-set-style internally in this way whilst initializing a buffer; if cc-set-style is called like this from anywhere else, it will usually behave as -a null operation." +a null operation. + +If DONT-OVERRIDE is the symbol `defaulting', style variables are set in such +a way that later calls to `c-set-style' will unconditionally override them +again, even if called with DONT-OVERRIDE t. This is is used when setting +style variables to the `c-default-style' early in initialization." (interactive (list (let ((completion-ignore-case t) (prompt (format "Which %s indentation style? " @@ -373,7 +422,8 @@ (error "Argument to c-set-style was not a string")) (c-initialize-builtin-style) (let ((vars (c-get-style-variables stylename nil))) - (unless dont-override + (unless (and dont-override + (not (eq dont-override 'defaulting))) ;; Since we always add to c-special-indent-hook we must reset it ;; first, or else the hooks from the preceding style will ;; remain. This is not necessary for c-offsets-alist, since @@ -640,15 +690,17 @@ (let ((func (if this-buf-only-p 'make-local-variable 'make-variable-buffer-local)) - (varsyms (cons 'c-indentation-style (copy-alist c-style-variables)))) + (varsyms (append '(c-indentation-style + c-style-defaulted-variables + c-style-defaulted-offsets) + (copy-alist c-style-variables)))) (delq 'c-special-indent-hook varsyms) (mapc func varsyms) ;; Hooks must be handled specially (if this-buf-only-p (if (featurep 'xemacs) (make-local-hook 'c-special-indent-hook)) (with-no-warnings (make-variable-buffer-local 'c-special-indent-hook)) - (setq c-style-variables-are-local-p t)) - )) + (setq c-style-variables-are-local-p t)))) -- NULL && (void)