[RFC PATCH] setting indentation styles via `c-file-style' fails to actually change indentation

[Nix]

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  <address@hidden>

        * 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)