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

[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  <nix@esperi.org.uk>

	* 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.")
+
 \f
 ;; 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))))
 
 
 \f


-- 
NULL && (void)

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

[Stefan Monnier]

> Certainly the state of play before this change works worse, but that's
> about as confident as I'm willing to be.

Thanks for taking the trouble to track down your problem and provide
a fix.  I'll let Alan check the actual change and decide if it's
safe enough.

But I just want to take the opportunity to say that all this CC style
business is much too complex for its own sake, and as a result we've
seen and keep seeing new problems that require additional hacks on top
of hacks to try and "fix" things.  I don't have the time/energy to
devote to cleaning this up, but it really needs a rethink and redesign
so that we can simplify the whole thing, thus removing warts rather than
working around them.


        Stefan

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

[Glenn Morris]

Nix wrote:

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

Previously reported as

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7570

(I think; I did not check very thoroughly.)

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

[Juanma Barranquero]

> Previously reported as
>
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7570

Yes, and as bug#8251, which was merged into #7570.

    Juanma

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

[Nix]

On 18 May 2011, Stefan Monnier outgrape:
> But I just want to take the opportunity to say that all this CC style
> business is much too complex for its own sake, and as a result we've
> seen and keep seeing new problems that require additional hacks on top
> of hacks to try and "fix" things.

Gods, yes. Most of this appears to be the interaction of backward-
compatibility kludge atop backward-compatibility kludge.

The indentation engine works great (the most nearly flawless I've ever
seen), and the style engine needs a certain degree of complexity --
things like inherited styles are a must, because so many styles in the
real world are almost like one of cc-mode's except for a couple of tiny
differences.

But the support for configuration through global variables *or* styles,
with styles possibly taking effect by localizing variables in one buffer
or possibly taking effect by making them local to all buffers... well,
it's just ridiculous. Rip out all that stuff and use the recommended
configuration only (style variables automatically localized to the
current buffer, direct setting of style variables always overrides the
style itself via 'set-from-style) and ditch all the rest. I'm fairly
sure that half the configurations implied by the current set of
variables are broken at any one time, because there are so many that
nobody can really test them.

(That's an alternative: a decent testing framework for this stuff, such
that we could be sure it didn't break. Then it wouldn't matter so much
how complex it got, except insofar as it bends the brains of people
trying to look at the code.)

-- 
NULL && (void)

ERT indentation testing (was: [RFC PATCH] setting indentation styles via `c-file-style' fails to actually change indentation)

[Ted Zlatanov]

On Wed, 18 May 2011 21:28:34 +0100 Nix <nix@esperi.org.uk> wrote: 

N> (That's an alternative: a decent testing framework for this stuff, such
N> that we could be sure it didn't break. Then it wouldn't matter so much
N> how complex it got, except insofar as it bends the brains of people
N> trying to look at the code.)

This can be done with ERT, right?  A bunch of .cc files with file-local
variables could be indented "properly" for those variables.  Then the
test logic for each file is 

1) open file and evaluate file-local variables, 
2) reindent and untabify the whole file, 
3) compare the result to the untabified original file.

The nice thing about this approach is that all the test logic is inside
the .cc file, so you don't need to configure the ELisp code for each
file separately.  To add a new test you just add a new .cc file.

Plus, of course, it's not limited to .cc files.  It would work just as
well for .c, .pl, .el, whatever.  I can write the ERT wrapper if this
sounds useful.

Ted

Re: ERT indentation testing

[Stefan Monnier]

> This can be done with ERT, right?  A bunch of .cc files with file-local
> variables could be indented "properly" for those variables.  Then the
> test logic for each file is 
[...]
> Plus, of course, it's not limited to .cc files.  It would work just as
> well for .c, .pl, .el, whatever.

Yup, we already have such files in test/indent.

> I can write the ERT wrapper if this sounds useful.

If you could integrate test/indent with ERT's automated tests, that
would be nice.


        Stefan

Re: ERT indentation testing

[Ted Zlatanov]

On Wed, 18 May 2011 19:19:25 -0300 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> This can be done with ERT, right?  A bunch of .cc files with file-local
>> variables could be indented "properly" for those variables.  Then the
>> test logic for each file is 
SM> [...]
>> Plus, of course, it's not limited to .cc files.  It would work just as
>> well for .c, .pl, .el, whatever.

SM> Yup, we already have such files in test/indent.

Yes, I see that now (I didn't see the commit for those, cool!)

We should have .c and .cc files with the major indentation styles
specified as file-local variables, plus .ini, Perl, Java, Python, and of
course Lisp (CL and ELisp).  Should I add those?

>> I can write the ERT wrapper if this sounds useful.

SM> If you could integrate test/indent with ERT's automated tests, that
SM> would be nice.

OK, I'll put it on my TODO list.

Ted

Re: ERT indentation testing

[Stefan Monnier]

>>> This can be done with ERT, right?  A bunch of .cc files with file-local
>>> variables could be indented "properly" for those variables.  Then the
>>> test logic for each file is 
SM> [...]
>>> Plus, of course, it's not limited to .cc files.  It would work just as
>>> well for .c, .pl, .el, whatever.

SM> Yup, we already have such files in test/indent.

> Yes, I see that now (I didn't see the commit for those, cool!)

> We should have .c and .cc files with the major indentation styles
> specified as file-local variables, plus .ini, Perl, Java, Python, and of
> course Lisp (CL and ELisp).  Should I add those?

Feel free.  I only added the ones I actually worked on.  BTW, the
octave.m testcase is too long for that kind of test (although the length
was instrumental in fixing some performance bugs).


        Stefan

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

[Nix]

On 18 May 2011, Stefan Monnier said:

>> Certainly the state of play before this change works worse, but that's
>> about as confident as I'm willing to be.
>
> Thanks for taking the trouble to track down your problem and provide
> a fix.  I'll let Alan check the actual change and decide if it's
> safe enough.

Ping?

-- 
NULL && (void)

Re: ERT indentation testing

[Ted Zlatanov]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On Thu, 19 May 2011 08:56:07 -0300 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>>>> This can be done with ERT, right?  A bunch of .cc files with file-local
>>>> variables could be indented "properly" for those variables.  Then the
>>>> test logic for each file is 
SM> [...]
>>>> Plus, of course, it's not limited to .cc files.  It would work just as
>>>> well for .c, .pl, .el, whatever.

SM> Yup, we already have such files in test/indent.

>> Yes, I see that now (I didn't see the commit for those, cool!)

>> We should have .c and .cc files with the major indentation styles
>> specified as file-local variables, plus .ini, Perl, Java, Python, and of
>> course Lisp (CL and ELisp).  Should I add those?

SM> Feel free.  I only added the ones I actually worked on.  BTW, the
SM> octave.m testcase is too long for that kind of test (although the length
SM> was instrumental in fixing some performance bugs).

See the attached patch to make the indentation testing use ERT and
operate on any files with a "." in the name except test-indent.el.

The test fails on modula2.mod and I'm not sure why.  I am setting
`enable-local-variables'.  Is it because I'm going line by line?  Or
something with the line boundaries?

Should the test stop when any file fails or keep going?

Thanks
Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test-indent.patch --]
[-- Type: text/x-diff, Size: 2729 bytes --]

=== modified file 'test/indent/Makefile'
--- test/indent/Makefile	2010-08-30 20:34:52 +0000
+++ test/indent/Makefile	2011-06-01 20:48:21 +0000
@@ -13,3 +13,6 @@
 	    --eval '(indent-region (point-min) (point-max) nil)' \
 	    --eval '(write-region (point-min) (point-max) "$<.new")'
 	diff -u -B $< $<.new
+
+test-indent:
+	$(EMACS) --batch -l ert -l test-indent.el -f ert-run-tests-batch-and-exit

=== added file 'test/indent/test-indent.el'
--- test/indent/test-indent.el	1970-01-01 00:00:00 +0000
+++ test/indent/test-indent.el	2011-06-01 21:28:07 +0000
@@ -0,0 +1,57 @@
+;;; test-indent.el --- run indentation tests
+
+;; Copyright (C) 2011  Free Software Foundation, Inc.
+
+;; Author: Ted Zlatanov <tzz@lifelogs.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The purpose of this module is to verify that all the files in te
+;; current directory are indented correctly.
+
+;;; Code:
+
+(eval-when-compile
+  (require 'ert)
+  (require 'cl))
+
+(ert-deftest test-indent-all ()
+  (let ((enable-local-variables :all)
+        lnum)
+    (loop for f in (delete "test-indent.el"
+                           (directory-files "." nil "^[^.]+\\."))
+          do (with-temp-buffer
+               (message "Testing indentation of %s" f)
+               (insert-file-contents f)
+               (goto-char (point-min))
+               (setq lnum 0)
+               (while (not (eobp))
+                 (incf lnum)
+                 (let* ((a (line-beginning-position))
+                        (b (line-end-position))
+                        (line (buffer-substring a b)))
+                   (message "Testing indentation of %s:%05d: %s" f lnum line)
+                   (unless (string-match "KNOWN INDENT BUG" line)
+                     (indent-region a b nil)
+                     (should (equal line (buffer-substring
+                                          (line-beginning-position)
+                                          (line-end-position)))))
+                   (forward-line)))))))
+
+(provide 'test-indent)
+;;; test-indent.el ends here

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

[Alan Mackenzie]

Hi, Nix.

On Wed, Jun 01, 2011 at 03:31:16PM +0100, Nix wrote:
> On 18 May 2011, Stefan Monnier said:

> >> Certainly the state of play before this change works worse, but that's
> >> about as confident as I'm willing to be.

> > Thanks for taking the trouble to track down your problem and provide
> > a fix.  I'll let Alan check the actual change and decide if it's
> > safe enough.

> Ping?

Sorry, I wasn't aware of this bug.  Thanks for the ping.  I'm not
subscribed to emacs-devel at the moment, but I'm still attending to
bug-cc-mode@gnu.org.

I'm now looking at the bug.  You didn't say what version of Emacs you
found it in, but I can say that this bug isn't in 23.3 or the standalone
CC Mode - at least not with `c-file-style' being used, in accordance with
its documentation, as a file local variable.

I agree with Stefan, the whole business of initialising style variables
has passed the level of complexity at which humans can handle it.  IMAO,
the straw that broke the camel's back was the misuse of `c-file-style' in
.dir-locals (for which I accept I'm mostly to blame).  Before any other
ways of simplification are explored, this use of `c-file-style' needs to
be separated from the normal use, possibly by something like this in the
Emacs core:

    (if (and (boundp 'c-file-style)
             c-file-style)
        (setq c-dir-style c-file-style
	      c-file-style nil))

.  Then CC Mode could get rid of all the attempted hacks for handling the
hybrid `c-file-style'.

At a guess, the bug is caused by calling c-file-style with a setting of
`dont-override' which causes it to choke on the above hacks.

Thanks for taking so much trouble to track this bug down.

> -- 
> NULL && (void)

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

> I agree with Stefan, the whole business of initialising style variables
> has passed the level of complexity at which humans can handle it.  IMAO,
> the straw that broke the camel's back was the misuse of `c-file-style' in
> .dir-locals (for which I accept I'm mostly to blame).  Before any other
> ways of simplification are explored, this use of `c-file-style' needs to
> be separated from the normal use, possibly by something like this in the
> Emacs core:

>     (if (and (boundp 'c-file-style)
>              c-file-style)
>         (setq c-dir-style c-file-style
> 	      c-file-style nil))

> Then CC Mode could get rid of all the attempted hacks for handling the
> hybrid `c-file-style'.

I'd rather not hardcode such CC-mode-specific code in Emacs core, but
maybe we can provide a generic way to do that or something similar.
The direct way to do the above would be for Emacs to provide
a "dir-locals-redirect" property which you could set on c-file-style,
redirecting it to c-dir-style.  Or maybe it would be enough to provide
both the list of file-local and the list of dir-local variables, so
that CC-mode could tell whether c-file-style was set file-locally, or
dir-locally, or some other way.


        Stefan

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

[Nix]

On 2 Jun 2011, Alan Mackenzie told this:
> At a guess, the bug is caused by calling c-file-style with a setting of
> `dont-override' which causes it to choke on the above hacks.

Er, yes. Did you miss my original post? 'cos I came to that conclusion,
and have a patch, which works for me, at least... but it's rather ugly,
so if there is a better way which would still allow setting of styles on
file-by-file and directory-(class)-by-directory-(class) basis, then I'm
glad to try that instead!

-- 
NULL && (void)

Re: ERT indentation testing

[Ted Zlatanov]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

On Wed, 01 Jun 2011 16:30:56 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> See the attached patch to make the indentation testing use ERT and
TZ> operate on any files with a "." in the name except test-indent.el.

Second patch:

TZ> The test fails on modula2.mod and I'm not sure why.  I am setting
TZ> `enable-local-variables'.  Is it because I'm going line by line?  Or
TZ> something with the line boundaries?

Uses `find-file' to process the local variables correctly.

TZ> Should the test stop when any file fails or keep going?

One test is created per file now, and any one file won't block the other tests.

Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test-indent.patch --]
[-- Type: text/x-diff, Size: 3376 bytes --]

=== modified file 'test/indent/Makefile'
--- test/indent/Makefile	2010-08-30 20:34:52 +0000
+++ test/indent/Makefile	2011-06-02 18:07:15 +0000
@@ -13,3 +13,6 @@
 	    --eval '(indent-region (point-min) (point-max) nil)' \
 	    --eval '(write-region (point-min) (point-max) "$<.new")'
 	diff -u -B $< $<.new
+
+test-indent:
+	$(EMACS) --batch -l ert -l test-indent.el -f ert-run-tests-batch-and-exit

=== added file 'test/indent/test-indent.el'
--- test/indent/test-indent.el	1970-01-01 00:00:00 +0000
+++ test/indent/test-indent.el	2011-06-02 18:39:23 +0000
@@ -0,0 +1,67 @@
+;;; test-indent.el --- run indentation tests
+
+;; Copyright (C) 2011  Free Software Foundation, Inc.
+
+;; Author: Ted Zlatanov <tzz@lifelogs.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The purpose of this module is to verify that all the files in te
+;; current directory are indented correctly.
+
+;;; Code:
+
+(eval-when-compile
+  (require 'ert)
+  (require 'cl)
+
+  (defsubst test-indent-trim (string)
+    "Lose leading and trailing whitespace.  Copied from BBDB."
+    (if (string-match "\\`[ \t\n]+" string)
+        (setq string (substring string (match-end 0))))
+    (if (string-match "[ \t\n]+\\'" string)
+        (setq string (substring string 0 (match-beginning 0))))
+    string)
+
+  (let ((files (delete "test-indent.el"
+                       (directory-files "." nil "\\`[^.]+\\..+[a-zA-Z]\\'")))
+        f)
+    (loop for i from 1 to (length files)
+          for f = (nth (1- i) files)
+          do (eval `(ert-deftest ,(intern (format "test-indent-%d-%s" i f)) ()
+                      (let ((lnum 0))
+                        (message "Testing indentation of %s" ,f)
+                        (find-file ,f)
+                        (goto-char (point-min))
+                        (while (not (eobp))
+                          (incf lnum)
+                          (let* ((a (line-beginning-position))
+                                 (b (line-end-position))
+                                 (line (buffer-substring a b)))
+                            (message "Testing indentation of %s:%05d: %s"
+                                     ,f lnum line)
+                            (unless (or (string-match "KNOWN INDENT BUG" line)
+                                        (equal (test-indent-trim line) ""))
+                              (indent-region a b nil)
+                              (should (equal line (buffer-substring
+                                                   (line-beginning-position)
+                                                   (line-end-position)))))
+                            (forward-line)))))))))
+
+(provide 'test-indent)
+;;; test-indent.el ends here

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

[Alan Mackenzie]

Hi, Nix.

On Thu, Jun 02, 2011 at 05:47:16PM +0100, Nix wrote:
> On 2 Jun 2011, Alan Mackenzie told this:
> > At a guess, the bug is caused by calling c-file-style with a setting of
> > `dont-override' which causes it to choke on the above hacks.

> Er, yes. Did you miss my original post? 'cos I came to that conclusion,

Sorry, I got a bit confused by the length of your post.

> and have a patch, which works for me, at least... but it's rather ugly,
> so if there is a better way which would still allow setting of styles on
> file-by-file and directory-(class)-by-directory-(class) basis, then I'm
> glad to try that instead!

I'm concerned that adding the extra value 'default to dont-override, and
also the two extra variables, might be an unwarranted increase in
complexity.

But first, I'd like to understand exactly what the bug is.  For a start,
_please_ tell me which version of Emacs you're testing with.  Anyhow,
...

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

I think what you're saying is that in a file like this,

    int main (int argc, char *argv[])
    {
        printf ("Hello, world!\n") ;
    }
    /* Local Variables: */
    /* c-file-style : "linux" */
    /* End:             */

, c-basic-offset remains 4, when it ought to become 8.  Is this in fact
the bug you have found?

Just one thing: are you absolutely sure you got the syntax of your Local
Variables: section correct?  If not, I think Emacs just fails to
recognise it without giving an error message.

[ ... ]

> 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, .....

Critical here is what value of `dont-override' is used in each of these
calls.

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

That is true.  The question is why is the c-set-style for the
`c-file-style' not doing its job?.  Normally it would override the
default style just set up.

Anyhow, first I need to be able to reproduce the bug.  Any help you can
give me with this will be most appreciated.

> -- 
> NULL && (void)

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Nix]

On 2 Jun 2011, Alan Mackenzie stated:

> Hi, Nix.
>
> On Thu, Jun 02, 2011 at 05:47:16PM +0100, Nix wrote:
>> On 2 Jun 2011, Alan Mackenzie told this:
>> > At a guess, the bug is caused by calling c-file-style with a setting of
>> > `dont-override' which causes it to choke on the above hacks.
>
>> Er, yes. Did you miss my original post? 'cos I came to that conclusion,
>
> Sorry, I got a bit confused by the length of your post.

That's fine, I had trouble understanding it myself :)

>> and have a patch, which works for me, at least... but it's rather ugly,
>> so if there is a better way which would still allow setting of styles on
>> file-by-file and directory-(class)-by-directory-(class) basis, then I'm
>> glad to try that instead!
>
> I'm concerned that adding the extra value 'default to dont-override, and
> also the two extra variables, might be an unwarranted increase in
> complexity.

Yes :( but every other solution seemed to be even *more* complex.

> But first, I'd like to understand exactly what the bug is.  For a start,
> _please_ tell me which version of Emacs you're testing with.  Anyhow,
> ...

This happens with trunk Emacs (24.1-to-be).

>> 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.
>
> I think what you're saying is that in a file like this,
>
>     int main (int argc, char *argv[])
>     {
>         printf ("Hello, world!\n") ;
>     }
>     /* Local Variables: */
>     /* c-file-style : "linux" */
>     /* End:             */
>
> , c-basic-offset remains 4, when it ought to become 8.  Is this in fact
> the bug you have found?

Not quite. In a file like this:

int main (int argc, char *argv[])
{
    printf ("Hello, world!\n") ;
}

with this style in effect:

(c-add-style "otbs" '("bsd" (c-offsets-alist . ((statement-cont . 4)
                                                (inextern-lang . 0)
                                                (label . 0)
                                                (arglist-cont-nonempty . 4))))))

and with directory local variables set (in my case) via

(dir-locals-set-class-variables 'unix '((c-mode . ((c-file-style . "otbs")
                                                   (fill-column . 80)
                                                   (indent-tabs-mode . t)))))

and the file assigned to the 'unix directory class, not a single one of
the 'otbs style elements take effect. Neither do the inherited 'bsd
elements. This differs from your case firstly because it tests style
inheritance, which is all tangled up with this, and secondly because it
tests dir local variables, which are where the breakage is visible.

> Just one thing: are you absolutely sure you got the syntax of your Local
> Variables: section correct?  If not, I think Emacs just fails to
> recognise it without giving an error message.

I can never actually remember the syntax of that section, because I
hardly ever use it: it's rather unfriendly when working in projects
where most members don't use Emacs, and apt to be inconsistent as files
you do not create don't have any of the variables set. Directory local
variables solve both these problems :)

> [ ... ]
>> 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, .....
>
> Critical here is what value of `dont-override' is used in each of these
> calls.

Well, it's called once at initialization time with the c-default-style
with dont-override set to 't' iff, oh, gods, you can read that code as
well as I can and I'm too smashed on antihistamines to parse it right
now. Then it gets called again in `c-before-hack-hook' with
dont-override set to 't' iff the only c-file-style was in the directory
local variable. (The rationale for this remains opaque to me, but since
the underlying bug in my case was in the first part I didn't investigate
that bit too closely. The style doesn't end up set to the directory-
local style: it ends up set to the default!)

Plus, each of these calls the underlying style-addition function
repeatedly for inherited styles.

>> ... 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.
>
> That is true.  The question is why is the c-set-style for the
> `c-file-style' not doing its job?.  Normally it would override the
> default style just set up.

It doesn't :( again, the email describes it more clearly than I can in
my current hayfever-wrecked state.

> Anyhow, first I need to be able to reproduce the bug.  Any help you can
> give me with this will be most appreciated.

Maybe the pollen count will go down soon and I can think about this
more clearly...

-- 
NULL && (void)

Re: ERT indentation testing

[Stefan Monnier]

> Uses `find-file' to process the local variables correctly.

Never call `find-file' from Elisp.  Always call
find-file-noselect instead.


        Stefan

Re: ERT indentation testing

[Ted Zlatanov]

On Thu, 02 Jun 2011 21:04:59 -0300 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> Uses `find-file' to process the local variables correctly.

SM> Never call `find-file' from Elisp.  Always call
SM> find-file-noselect instead.

I was trying to copy your original test, which I think uses `find-file':

#+begin_src makefile
%.test: %
	-$(RM) $<.new
	$(EMACS) --batch $< \
	    --eval '(indent-region (point-min) (point-max) nil)' \
	    --eval '(write-region (point-min) (point-max) "$<.new")'
	diff -u -B $< $<.new

#+end_src

Using `find-file-noselect' will just make me select the buffer in ELisp
instead of expecting it to become current.  Is there any real reason to
avoid `find-file'?  Because of the prompts?  I ask because I'm curious;
I've already changed my patch to use (set-buffer (find-file-noselect...))
but can't commit because the Bazaar repo is down.

Also, if `find-file' should be avoided in ELisp generally, the docstring
should say so, like it does for many other functions.

Actually it might be nice if there was a standard way to tag such
extra-interactive functions that are not supposed to be called from an
ELisp context, only from `M-x' or through a key.  A symbol property
maybe?  Then the compiler could check it and print suitable errors...
Tell me it already exists :)

Thanks
Ted

Re: ERT indentation testing

[martin rudalics]

 > Using `find-file-noselect' will just make me select the buffer in ELisp
 > instead of expecting it to become current.  Is there any real reason to
 > avoid `find-file'?  Because of the prompts?  I ask because I'm curious;
 > I've already changed my patch to use (set-buffer (find-file-noselect...))
 > but can't commit because the Bazaar repo is down.

`find-file' uses `switch-to-buffer' and `switch-to-buffer' should be
avoided in Elisp code.

 > Also, if `find-file' should be avoided in ELisp generally, the docstring
 > should say so, like it does for many other functions.

Indeed.

martin

extra-interactive functions (was: ERT indentation testing)

[Ted Zlatanov]

On Fri, 03 Jun 2011 14:12:24 +0200 martin rudalics <rudalics@gmx.at> wrote: 

>> Using `find-file-noselect' will just make me select the buffer in ELisp
>> instead of expecting it to become current.  Is there any real reason to
>> avoid `find-file'?  Because of the prompts?  I ask because I'm curious;
>> I've already changed my patch to use (set-buffer (find-file-noselect...))
>> but can't commit because the Bazaar repo is down.

mr> `find-file' uses `switch-to-buffer' and `switch-to-buffer' should be
mr> avoided in Elisp code.

The docstring for `switch-to-buffer' says not to use it to avoid
"messing with the window-buffer correspondences" which is good enough
for me (though I'm still curious what that really means).

>> Also, if `find-file' should be avoided in ELisp generally, the docstring
>> should say so, like it does for many other functions.

mr> Indeed.

But that's tedious to do by hand.  If we tag the function symbol
(e.g. `switch-to-buffer') with some extra-interactive property, we can
tag all the functions that call it as well at compile time.  Wouldn't
that be nicer?  The docstring can then automatically say "unsafe to call
because it calls `switch-to-buffer'" which is nice and more helpful.

Ted

Re: extra-interactive functions

[martin rudalics]

 > mr> `find-file' uses `switch-to-buffer' and `switch-to-buffer' should be
 > mr> avoided in Elisp code.
 >
 > The docstring for `switch-to-buffer' says not to use it to avoid
 > "messing with the window-buffer correspondences" which is good enough
 > for me (though I'm still curious what that really means).

A user might want to display that file visting buffer in a certain way
and has customized options like `special-display-buffer-names' or
`special-display-regexps' for that purpose.  If you call
`switch-to-buffer', the user cannot control the placement of the buffer
in the window or frame of her choice.

 >>> Also, if `find-file' should be avoided in ELisp generally, the docstring
 >>> should say so, like it does for many other functions.
 >
 > mr> Indeed.
 >
 > But that's tedious to do by hand.  If we tag the function symbol
 > (e.g. `switch-to-buffer') with some extra-interactive property, we can
 > tag all the functions that call it as well at compile time.  Wouldn't
 > that be nicer?  The docstring can then automatically say "unsafe to call
 > because it calls `switch-to-buffer'" which is nice and more helpful.

There are only very few interactive functions that should be allowed to
call `switch-to-buffer'.  `find-file' may fit into this category because
that's what users traditionally expect it to do (although I plan to have
it call a function `pop-to-buffer-same-window' sooner or later).  In the
distant future there should be no function calling `switch-to-buffer' at
all.

martin

Re: extra-interactive functions

[Ted Zlatanov]

On Fri, 03 Jun 2011 17:27:45 +0200 martin rudalics <rudalics@gmx.at> wrote: 

>>>> Also, if `find-file' should be avoided in ELisp generally, the docstring
>>>> should say so, like it does for many other functions.
>> 
mr> Indeed.
>> 
>> But that's tedious to do by hand.  If we tag the function symbol
>> (e.g. `switch-to-buffer') with some extra-interactive property, we can
>> tag all the functions that call it as well at compile time.  Wouldn't
>> that be nicer?  The docstring can then automatically say "unsafe to call
>> because it calls `switch-to-buffer'" which is nice and more helpful.

mr> There are only very few interactive functions that should be allowed to
mr> call `switch-to-buffer'.  `find-file' may fit into this category because
mr> that's what users traditionally expect it to do (although I plan to have
mr> it call a function `pop-to-buffer-same-window' sooner or later).  In the
mr> distant future there should be no function calling `switch-to-buffer' at
mr> all.

Thank you for explaining.

You're sort of side-stepping my suggestion of a special tag for
functions that should not be called from ELisp.  Is it because you think
it's too unusual to be useful, or something else?

Ted

Re: extra-interactive functions

[martin rudalics]

 > You're sort of side-stepping my suggestion of a special tag for
 > functions that should not be called from ELisp.  Is it because you think
 > it's too unusual to be useful, or something else?

Such functions should be in `byte-compile-interactive-only-functions'.
Currently, I don't want to include `switch-to-buffer' because it would
create too many useless warnings.  Eventually, I have to check in every
single case whether the intended use is to really show the buffer in the
selected window or simply in some window.

We could add `find-file' though ;-)

martin

Re: extra-interactive functions

[Ted Zlatanov]

On Fri, 03 Jun 2011 20:59:29 +0200 martin rudalics <rudalics@gmx.at> wrote: 

>> You're sort of side-stepping my suggestion of a special tag for
>> functions that should not be called from ELisp.  Is it because you think
>> it's too unusual to be useful, or something else?

mr> Such functions should be in `byte-compile-interactive-only-functions'.

Oh cool, so it already exists :) Can the docstring for those functions
automatically include a warning about usage from ELisp?  Since you're so
careful about including functions in that list, it makes sense to use it
authoritatively.

mr> Currently, I don't want to include `switch-to-buffer' because it would
mr> create too many useless warnings.  Eventually, I have to check in every
mr> single case whether the intended use is to really show the buffer in the
mr> selected window or simply in some window.

mr> We could add `find-file' though ;-)

Works for me.

Ted

Re: extra-interactive functions

[martin rudalics]

 > mr> Such functions should be in `byte-compile-interactive-only-functions'.
 >
 > Oh cool, so it already exists :) Can the docstring for those functions
 > automatically include a warning about usage from ELisp?

I suppose that anyone adding a function to that list should take care of
fixing the doc-string appropriately.

 > Since you're so
 > careful about including functions in that list,

Am I?

 > it makes sense to use it
 > authoritatively.

IMHO the warnings are sufficiently annyoing during compilation.  Just
the authors don't seem very annoyed ;-)

martin

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

[Alan Mackenzie]

Hi, Nix.

On Thu, Jun 02, 2011 at 10:43:06PM +0100, Nix wrote:
> On 2 Jun 2011, Alan Mackenzie stated:

> > On Thu, Jun 02, 2011 at 05:47:16PM +0100, Nix wrote:
> >> On 2 Jun 2011, Alan Mackenzie told this:

> >> and have a patch, which works for me, at least... but it's rather ugly,
> >> so if there is a better way which would still allow setting of styles on
> >> file-by-file and directory-(class)-by-directory-(class) basis, then I'm
> >> glad to try that instead!

OK.  Would you please try this patch and let me know how it works.  It's
a bit less ugly than your one (-:


*** cc-mode.el.~4~	2011-06-04 10:32:15.000000000 +0000
--- cc-mode.el	2011-06-05 12:05:12.000000000 +0000
***************
*** 692,698 ****
  		   (c-count-cfss file-local-variables-alist))
  		  (cfs-in-dir-count (c-count-cfss dir-local-variables-alist)))
  	      (c-set-style stile
! 			   (= cfs-in-file-and-dir-count cfs-in-dir-count)))
  	  (c-set-style stile)))
        (when offsets
  	(mapc
--- 692,699 ----
  		   (c-count-cfss file-local-variables-alist))
  		  (cfs-in-dir-count (c-count-cfss dir-local-variables-alist)))
  	      (c-set-style stile
! 			   (and (= cfs-in-file-and-dir-count cfs-in-dir-count)
! 				'keep-defaults)))
  	  (c-set-style stile)))
        (when offsets
  	(mapc


[ BTW, you don't need to separate the patch out of the email.  Just give
patch the entire mail, and it will cope.]


> This happens with trunk Emacs (24.1-to-be).

OK, thanks.  That's the version the above patch applies to.

****************************************************************************

> Not quite. In a file like this:

> int main (int argc, char *argv[])
> {
>     printf ("Hello, world!\n") ;
> }

> with this style in effect:

> (c-add-style "otbs" '("bsd" (c-offsets-alist . ((statement-cont . 4)
>                                                 (inextern-lang . 0)
>                                                 (label . 0)
>                                                 (arglist-cont-nonempty . 4))))))

> and with directory local variables set (in my case) via

> (dir-locals-set-class-variables 'unix '((c-mode . ((c-file-style . "otbs")
>                                                    (fill-column . 80)
>                                                    (indent-tabs-mode . t)))))

> and the file assigned to the 'unix directory class, not a single one of
> the 'otbs style elements take effect. Neither do the inherited 'bsd
> elements. This differs from your case firstly because it tests style
> inheritance, which is all tangled up with this, and secondly because it
> tests dir local variables, which are where the breakage is visible.

OK, thanks.  That's what I needed to know.

> > [ ... ]
> >> 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, .....

> > Critical here is what value of `dont-override' is used in each of these
> > calls.

> Well, it's called once at initialization time with the c-default-style
> with dont-override set to 't' iff, oh, gods, you can read that code as
> well as I can .....

I've had several years practice, and I wrote some of it.  ;-)

> ..... and I'm too smashed on antihistamines to parse it right now.

Sorry to hear that.

> Then it gets called again in `c-before-hack-hook' with dont-override
> set to 't' iff the only c-file-style was in the directory local
> variable. (The rationale for this remains opaque to me, but since the
> underlying bug in my case was in the first part I didn't investigate
> that bit too closely. The style doesn't end up set to the directory-
> local style: it ends up set to the default!)

It was an attempt to prevent .dir-local variables from overriding more
specifically set variables (e.g. set in a hook function).  It didn't work
very well.  :-(

> Plus, each of these calls the underlying style-addition function
> repeatedly for inherited styles.

Yes.  That seems safer than trying to separate out what sort of style
setting belongs where, and how to mix them when that's required.

> Maybe the pollen count will go down soon and I can think about this
> more clearly...

Let's hope so.

In the mean time, please forgive me offering you some free advice on bug
reports:

1. Say what you do to cause the bug, not merely what state a program is
in when the bug happens.

2. Be VERY wary about sending "streams of consciousness"; they can
sometimes be difficult to make sense of.  Reading them the morning after
can be helpful.

3. Don't assume the grizzled senile maintainer can simply follow a patch;
explain it!  Often the bug submitter, having just spent several hours on
it, knows the piece of code better than the maintainer.

4. In Emacs, the standard diff format is contextual (-c), but unified
(-u) is OK too.

So, let me know if the patch works, then we can close this bug.

> -- 
> NULL && (void)

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: ERT indentation testing

[Ted Zlatanov]

On Thu, 02 Jun 2011 13:43:27 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> On Wed, 01 Jun 2011 16:30:56 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 
TZ> See the attached patch to make the indentation testing use ERT and
TZ> operate on any files with a "." in the name except test-indent.el.

TZ> Second patch:

TZ> The test fails on modula2.mod and I'm not sure why.  I am setting
TZ> `enable-local-variables'.  Is it because I'm going line by line?  Or
TZ> something with the line boundaries?

TZ> Uses `find-file' to process the local variables correctly.

Is the patch above usable (using `find-file-noselect' as discussed)?
Should I keep working on this, commit it as is, or forget about it?

Thanks
Ted

Re: ERT indentation testing

[Stefan Monnier]

> Is the patch above usable (using `find-file-noselect' as discussed)?

I trust your judgment on it.  The functionality is one I'd like to see,
but I don't have time to check the actual code.

> Should I keep working on this, commit it as is, or forget about it?

If/when you think it's ready, please install it.


        Stefan

Re: ERT indentation testing

[Ted Zlatanov]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Wed, 08 Jun 2011 12:24:13 -0300 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> Is the patch above usable (using `find-file-noselect' as discussed)?
SM> I trust your judgment on it.  The functionality is one I'd like to see,
SM> but I don't have time to check the actual code.

OK, great.

>> Should I keep working on this, commit it as is, or forget about it?

SM> If/when you think it's ready, please install it.

I'm concerned that it's failing a few tests.  I think the files are
incorrect for the .rc and .sh content, don't know about the Prolog.

   FAILED  test-indent-2-prolog\.prolog
   FAILED  test-indent-3-shell\.rc
   FAILED  test-indent-4-shell\.sh

Third patch attached, just "make test-indent".  If you think those tests
are legitimate failures, I'll commit, but otherwise I should fix them.

Thanks
Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test-indent.patch --]
[-- Type: text/x-diff, Size: 3398 bytes --]

=== modified file 'test/indent/Makefile'
--- test/indent/Makefile	2010-08-30 20:34:52 +0000
+++ test/indent/Makefile	2011-06-02 18:07:15 +0000
@@ -13,3 +13,6 @@
 	    --eval '(indent-region (point-min) (point-max) nil)' \
 	    --eval '(write-region (point-min) (point-max) "$<.new")'
 	diff -u -B $< $<.new
+
+test-indent:
+	$(EMACS) --batch -l ert -l test-indent.el -f ert-run-tests-batch-and-exit

=== added file 'test/indent/test-indent.el'
--- test/indent/test-indent.el	1970-01-01 00:00:00 +0000
+++ test/indent/test-indent.el	2011-06-02 18:39:23 +0000
@@ -0,0 +1,67 @@
+;;; test-indent.el --- run indentation tests
+
+;; Copyright (C) 2011  Free Software Foundation, Inc.
+
+;; Author: Ted Zlatanov <tzz@lifelogs.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The purpose of this module is to verify that all the files in te
+;; current directory are indented correctly.
+
+;;; Code:
+
+(eval-when-compile
+  (require 'ert)
+  (require 'cl)
+
+  (defsubst test-indent-trim (string)
+    "Lose leading and trailing whitespace.  Copied from BBDB."
+    (if (string-match "\\`[ \t\n]+" string)
+        (setq string (substring string (match-end 0))))
+    (if (string-match "[ \t\n]+\\'" string)
+        (setq string (substring string 0 (match-beginning 0))))
+    string)
+
+  (let ((files (delete "test-indent.el"
+                       (directory-files "." nil "\\`[^.]+\\..+[a-zA-Z]\\'")))
+        f)
+    (loop for i from 1 to (length files)
+          for f = (nth (1- i) files)
+          do (eval `(ert-deftest ,(intern (format "test-indent-%d-%s" i f)) ()
+                      (let ((lnum 0))
+                        (message "Testing indentation of %s" ,f)
+                        (set-buffer (find-file-noselect ,f))
+                        (goto-char (point-min))
+                        (while (not (eobp))
+                          (incf lnum)
+                          (let* ((a (line-beginning-position))
+                                 (b (line-end-position))
+                                 (line (buffer-substring a b)))
+                            (message "Testing indentation of %s:%05d: %s"
+                                     ,f lnum line)
+                            (unless (or (string-match "KNOWN INDENT BUG" line)
+                                        (equal (test-indent-trim line) ""))
+                              (indent-region a b nil)
+                              (should (equal line (buffer-substring
+                                                   (line-beginning-position)
+                                                   (line-end-position)))))
+                            (forward-line)))))))))
+
+(provide 'test-indent)
+;;; test-indent.el ends here

Re: ERT indentation testing

[Stefan Monnier]

> I'm concerned that it's failing a few tests.  I think the files are
> incorrect for the .rc and .sh content, don't know about the Prolog.

>    FAILED  test-indent-2-prolog\.prolog
>    FAILED  test-indent-3-shell\.rc
>    FAILED  test-indent-4-shell\.sh

Check the current result of the test in test/indent.
If the same problems show up there, it's a sign those have nothing to do
with your changes,


        Stefan

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

[Kan-Ru Chen]

Thanks to Alan, this bug has been fixed in bzr r104815

-- 
Kanru

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

[Nix]

On 5 Jun 2011, Alan Mackenzie said:

> Hi, Nix.
>
> On Thu, Jun 02, 2011 at 10:43:06PM +0100, Nix wrote:
>> On 2 Jun 2011, Alan Mackenzie stated:
>
>> > On Thu, Jun 02, 2011 at 05:47:16PM +0100, Nix wrote:
>> >> On 2 Jun 2011, Alan Mackenzie told this:
>
>> >> and have a patch, which works for me, at least... but it's rather ugly,
>> >> so if there is a better way which would still allow setting of styles on
>> >> file-by-file and directory-(class)-by-directory-(class) basis, then I'm
>> >> glad to try that instead!
>
> OK.  Would you please try this patch and let me know how it works.  It's
> a bit less ugly than your one (-:

Sorry for the insane delay, a big system update meant that restarting
Emacs was inadvisable for a few weeks until I'd crushed some (non-Emacs)
bugs. Thankfully there were no power cuts in this period :)

Your fix (in the form you committed it, since I delayed so long) seems
to work, though despite a lot of looking at it I'm not quite sure how!
God knows it's less grotesque than the 'pile things in then filter them
out again' approach I was employing.

>> Then it gets called again in `c-before-hack-hook' with dont-override
>> set to 't' iff the only c-file-style was in the directory local
>> variable. (The rationale for this remains opaque to me, but since the
>> underlying bug in my case was in the first part I didn't investigate
>> that bit too closely. The style doesn't end up set to the directory-
>> local style: it ends up set to the default!)
>
> It was an attempt to prevent .dir-local variables from overriding more
> specifically set variables (e.g. set in a hook function).  It didn't work
> very well.  :-(

Yeah. There are arguments on both sides for whether hook functions
should override dir-local (and for that matter file-local) variables.
I'm not sure which way the priorities should run, but I'm fairly sure it
should be consistent for all of Emacs, not just cc-mode, which implies
that we need some sort of helper that everyone can call to disambiguate
these cases.

> 2. Be VERY wary about sending "streams of consciousness"; they can
> sometimes be difficult to make sense of.  Reading them the morning after
> can be helpful.

Yeah, sorry about that. It wasn't actually a stream of consciousness:
it was a victim of repeated over-editing for 'clarity' (for which read
incomprehensibility).

> 3. Don't assume the grizzled senile maintainer can simply follow a patch;
> explain it!  Often the bug submitter, having just spent several hours on
> it, knows the piece of code better than the maintainer.

OK. I've been castigated for over-explaining patches on other lists, and
even for commenting code: nice to know this list is a 'please explain
yourself' list, I much prefer that.

> 4. In Emacs, the standard diff format is contextual (-c), but unified
> (-u) is OK too.

I still don't understand how anyone can read context diffs. :) but then
other poeple find unified diffs impossible to read as well. Obviously
we should use the UK government standard format and send things around
as PDF files with the old text blacked out by inserting a black box
above it. ;}

> So, let me know if the patch works, then we can close this bug.

It works!

-- 
NULL && (void)