prog-mode not available in earlier Emacsen, need help with cfengine.el

prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

I'd like, instead of saying 

(define-derived-mode cfengine3-mode prog-mode "CFEngine3" ...

to say something like:

(define-derived-mode cfengine3-mode (if (fboundp 'prog-mode) prog-mode fundamental-mode) "CFEngine3" ...

or something like it, which will keep the compiler happy and will work
in older Emacsen.  I'd rather not maintain two versions of cfengine.el.
I don't use any prog-mode features in cfengine.el.

Is that possible and OK with the maintainers, and if so, can I have an
example which works in Emacs 23.x and 24.x alike (and, I hope, XEmacs)?

Thanks
Ted

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Eli Zaretskii]

> From: Ted Zlatanov <tzz@lifelogs.com>
> Date: Tue, 22 Nov 2011 12:18:10 -0500
> 
> I'd like, instead of saying 
> 
> (define-derived-mode cfengine3-mode prog-mode "CFEngine3" ...
> 
> to say something like:
> 
> (define-derived-mode cfengine3-mode (if (fboundp 'prog-mode) prog-mode fundamental-mode) "CFEngine3" ...
> 
> or something like it, which will keep the compiler happy and will work
> in older Emacsen.  I'd rather not maintain two versions of cfengine.el.
> I don't use any prog-mode features in cfengine.el.

Why can't you define a fake prog-mode in cfengine.el, if prog-mode is
not already fboundp?

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

On Tue, 22 Nov 2011 20:05:41 +0200 Eli Zaretskii <eliz@gnu.org> wrote: 

>> From: Ted Zlatanov <tzz@lifelogs.com>
>> Date: Tue, 22 Nov 2011 12:18:10 -0500
>> 
>> I'd like, instead of saying 
>> 
>> (define-derived-mode cfengine3-mode prog-mode "CFEngine3" ...
>> 
>> to say something like:
>> 
>> (define-derived-mode cfengine3-mode (if (fboundp 'prog-mode) prog-mode fundamental-mode) "CFEngine3" ...
>> 
>> or something like it, which will keep the compiler happy and will work
>> in older Emacsen.  I'd rather not maintain two versions of cfengine.el.
>> I don't use any prog-mode features in cfengine.el.

EZ> Why can't you define a fake prog-mode in cfengine.el, if prog-mode is
EZ> not already fboundp?

That's a good way, but I don't know if the maintainers would mind, and
if it would cause autoload/compilation issues.  Do I have to wrap it all
in `eval-when-compile'?

Ted

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Stefan Monnier]

> That's a good way, but I don't know if the maintainers would mind, and

Which maintainers?

> if it would cause autoload/compilation issues.  Do I have to wrap it all
> in `eval-when-compile'?

I think that

  (unless (fboundp 'prog-mode) (defalias 'prog-mode 'fundamental-mode))

should do the trick.


        Stefan

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

On Tue, 22 Nov 2011 17:30:01 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote: 

>> That's a good way, but I don't know if the maintainers would mind, and
SM> Which maintainers?

The Emacs maintainers.  I mean I don't know if you want to have
fallbacks like that in the Emacs 24 code, since obviously prog-mode
exists there.

>> if it would cause autoload/compilation issues.  Do I have to wrap it all
>> in `eval-when-compile'?

SM> I think that

SM>   (unless (fboundp 'prog-mode) (defalias 'prog-mode 'fundamental-mode))

SM> should do the trick.

The compilation passes cleanly and it seems to work OK under 24.  I'll
try it out in 23 and XEmacs.

Thanks
Ted

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Chong Yidong]

Ted Zlatanov <tzz@lifelogs.com> writes:

>>> That's a good way, but I don't know if the maintainers would mind, and
> SM> Which maintainers?
>
> The Emacs maintainers.  I mean I don't know if you want to have
> fallbacks like that in the Emacs 24 code, since obviously prog-mode
> exists there.

Compatibility code is OK for big packages like Tramp and Gnus,
especially those developed independently or semi-independently of Emacs.
But for stuff like cfengine.el, it's better to just track the Emacs
version, I think.

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

On Wed, 23 Nov 2011 14:03:04 +0800 Chong Yidong <cyd@gnu.org> wrote: 

CY> Ted Zlatanov <tzz@lifelogs.com> writes:
>>>> That's a good way, but I don't know if the maintainers would mind, and
SM> Which maintainers?
>> 
>> The Emacs maintainers.  I mean I don't know if you want to have
>> fallbacks like that in the Emacs 24 code, since obviously prog-mode
>> exists there.

CY> Compatibility code is OK for big packages like Tramp and Gnus,
CY> especially those developed independently or semi-independently of Emacs.
CY> But for stuff like cfengine.el, it's better to just track the Emacs
CY> version, I think.

I was planning on making the change in the Emacs version so it's in sync
with the other one (which lives in the cfengine SVN trunk under
contrib/).  Do you mean you would rather have two versions?

Ted

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Chong Yidong]

Ted Zlatanov <tzz@lifelogs.com> writes:

> I was planning on making the change in the Emacs version so it's in sync
> with the other one (which lives in the cfengine SVN trunk under
> contrib/).  Do you mean you would rather have two versions?

Yes.

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

On Thu, 24 Nov 2011 14:03:40 +0800 Chong Yidong <cyd@gnu.org> wrote: 

CY> Ted Zlatanov <tzz@lifelogs.com> writes:
>> I was planning on making the change in the Emacs version so it's in sync
>> with the other one (which lives in the cfengine SVN trunk under
>> contrib/).  Do you mean you would rather have two versions?

CY> Yes.

I'll change the non-Emacs one so it explains this and has compatibility
code.  Thank you for explaining.

Ted

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Reiner Steib]

On Fri, Nov 25 2011, Ted Zlatanov wrote:

| On Tue, Nov 22 2011, Stefan Monnier wrote:
| > I think that
| >   (unless (fboundp 'prog-mode) (defalias 'prog-mode 'fundamental-mode))
| > should do the trick.

> I'll change the non-Emacs one so it explains this and has compatibility
> code.  

Please don't define/defalias prog-mode, because other code might test
for (fboundp 'prog-mode) an expect a real prog-mode.

,----[ (info "(elisp)Coding Conventions") ]
|    * If a package needs to define an alias or a new function for
|      compatibility with some other version of Emacs, name it with the
|      package prefix, not with the raw name with which it occurs in the
|      other version.  Here is an example from Gnus, which provides many
|      examples of such compatibility issues.
| 
|           (defalias 'gnus-point-at-bol
|             (if (fboundp 'point-at-bol)
|                 'point-at-bol
|               'line-beginning-position))
`----

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

On Sat, 26 Nov 2011 11:57:36 +0100 Reiner Steib <reinersteib+gmane@imap.cc> wrote: 

RS> On Fri, Nov 25 2011, Ted Zlatanov wrote:
RS> | On Tue, Nov 22 2011, Stefan Monnier wrote:
RS> | > I think that
RS> | >   (unless (fboundp 'prog-mode) (defalias 'prog-mode 'fundamental-mode))
RS> | > should do the trick.

>> I'll change the non-Emacs one so it explains this and has compatibility
>> code.  

RS> Please don't define/defalias prog-mode, because other code might test
RS> for (fboundp 'prog-mode) an expect a real prog-mode.

RS> ,----[ (info "(elisp)Coding Conventions") ]
RS> |    * If a package needs to define an alias or a new function for
RS> |      compatibility with some other version of Emacs, name it with the
RS> |      package prefix, not with the raw name with which it occurs in the
RS> |      other version.  Here is an example from Gnus, which provides many
RS> |      examples of such compatibility issues.
RS> | 
RS> |           (defalias 'gnus-point-at-bol
RS> |             (if (fboundp 'point-at-bol)
RS> |                 'point-at-bol
RS> |               'line-beginning-position))
RS> `----

Understood, thanks!

Ted

Re: prog-mode not available in earlier Emacsen, need help with cfengine.el

[Ted Zlatanov]

On Sat, 26 Nov 2011 21:06:42 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> On Sat, 26 Nov 2011 11:57:36 +0100 Reiner Steib <reinersteib+gmane@imap.cc> wrote: 

RS> Please don't define/defalias prog-mode, because other code might test
RS> for (fboundp 'prog-mode) an expect a real prog-mode.

TZ> Understood, thanks!

I did this as suggested.

The cfengine3 project now has the compatibility version of cfengine.el,
tested back to Emacs 23.1 (btw, did you know you can't even compile 23.1
on Ubuntu 11?).  I'll keep the two in sync as much as possible and
encourage cfengine3 Emacs users to upgrade to Emacs 24 where
cfengine3-mode is built-in.

Thanks
Ted

changes to cfengine-mode (was: prog-mode not available in earlier Emacsen, need help with cfengine.el)

[Ted Zlatanov]

I'd like to rename `cfengine-mode' to `cfengine2-mode', and
`cfengine-auto-mode' to `cfengine-mode' (`cfengine-auto-mode' loads
`cfengine3-mode' if it thinks a cfengine v.3 file is edited).

The effect on users of CFengine v.2 will be nil, since the detection is
very cautious and assumes v.2.  Users of v.3 will get `cfengine3-mode'.

Another approach is to rename `cfengine-mode' to `cfengine2-mode' and
`cfengine3-mode' to `cfengine-mode', effectively making v.2 support less
important.  **I like this approach better** because cfengine v.2 is
disappearing quickly, but it would break backwards compatibility so I
proposed it second.

Let me know what you think.  Thanks!

Ted

Re: changes to cfengine-mode

[Stefan Monnier]

> I'd like to rename `cfengine-mode' to `cfengine2-mode', and
> `cfengine-auto-mode' to `cfengine-mode' (`cfengine-auto-mode' loads
> `cfengine3-mode' if it thinks a cfengine v.3 file is edited).

That sounds right.

> Another approach is to rename `cfengine-mode' to `cfengine2-mode' and
> `cfengine3-mode' to `cfengine-mode', effectively making v.2 support less
> important.  **I like this approach better** because cfengine v.2 is
> disappearing quickly, but it would break backwards compatibility so I
> proposed it second.

We can consider it for 24.2,


        Stefan

Re: changes to cfengine-mode

[Ted Zlatanov]

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

On Fri, 02 Dec 2011 15:32:25 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote: 

>> I'd like to rename `cfengine-mode' to `cfengine2-mode', and
>> `cfengine-auto-mode' to `cfengine-mode' (`cfengine-auto-mode' loads
>> `cfengine3-mode' if it thinks a cfengine v.3 file is edited).

SM> That sounds right.

Thanks for looking.  Attached is a patch that will:

1) rename `cfengine-mode' to `cfengine2-mode' and defalias
'cfengine-mode to 'cfengine-auto-mode.  Provide 'cfengine2.

2) rename all the "cfengine-*" variables used only by `cfengine2-mode'
to "cfengine2-*"

3) adjust the commentary to match

4) give a version and set it to 1.1 for this release

5) use "CFEngine" consistently, capitalized as the author, Mark Burgess,
does

6) add `cfengine3-mode-verbose' to control some debugging info

7) add `cfengine3-parameters-indent' to address some shortcomings in
indentation.  This is a new feature but necessary to fix the lack of a
hanging indent in the current `cfengine3-mode'.  It's very low-risk and
I hope it's acceptable.

8) improve and capitalize comments in many places, though that work is
not complete

9) change the "lighter" modeline indicators from "Cfengine[23]" to
"CFE[23]" to take less space

I also have a font-lock issue I can't figure out with the current
version.  Given the following code (use `cfengine3-mode' to see the
problem):

#+begin_src cfengine3
body common control
{
      bundlesequence => { "g", "cfrun", "rcfiles", "packages", "packages_push" };
      inputs => { "cfengine_stdlib.cf" };
}

bundle common g
{
}
#+end_src

These are two defuns as far as `cfengine3-mode' is concerned.

The "bundle common g" line is supposed to be font-locked and it is,
sometimes.  For instance it's not font-locked in a text buffer where
you've just run `cfengine3-mode', but if you remove the blank line
between the two defuns, it gets font-locked.  Then if I do more editing
it keeps flip-flopping between font-locked and undecorated.  I've spend
many hours looking for the cause of this issue and I hope you or someone
else will spot it more easily, or tell me how to debug it better.  I
have a feeling it's due to the defun motion commands but I'm not sure.

>> Another approach is to rename `cfengine-mode' to `cfengine2-mode' and
>> `cfengine3-mode' to `cfengine-mode', effectively making v.2 support less
>> important.  **I like this approach better** because cfengine v.2 is
>> disappearing quickly, but it would break backwards compatibility so I
>> proposed it second.

SM> We can consider it for 24.2,

OK.

Ted


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

=== modified file 'lisp/progmodes/cfengine.el'
--- lisp/progmodes/cfengine.el	2011-11-20 03:48:53 +0000
+++ lisp/progmodes/cfengine.el	2011-12-07 10:48:58 +0000
@@ -5,6 +5,9 @@
 ;; Author: Dave Love <fx@gnu.org>
 ;; Maintainer: Ted Zlatanov <tzz@lifelogs.com>
 ;; Keywords: languages
+;; Version: 1.1
+(defvar cfengine-version nil)
+(setq cfengine-version "1.1")
 
 ;; This file is part of GNU Emacs.
 
@@ -29,18 +32,18 @@
 ;; The CFEngine 3.x support doesn't have Imenu support but patches are
 ;; welcome.
 
-;; You can set it up so either cfengine-mode (2.x and earlier) or
-;; cfengine3-mode (3.x) will be picked, depending on the buffer
+;; You can set it up so either `cfengine2-mode' (2.x and earlier) or
+;; `cfengine3-mode' (3.x) will be picked, depending on the buffer
 ;; contents:
 
-;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-auto-mode))
+;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-mode))
 
 ;; OR you can choose to always use a specific version, if you prefer
-;; it
+;; it:
 
 ;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine3-mode))
-;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine-mode))
-;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine-mode))
+;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine2-mode))
+;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine2-mode))
 
 ;; This is not the same as the mode written by Rolf Ebert
 ;; <ebert@waporo.muc.de>, distributed with cfengine-2.0.5.  It does
@@ -49,16 +52,32 @@
 ;;; Code:
 
 (defgroup cfengine ()
-  "Editing Cfengine files."
+  "Editing CFEngine files."
   :group 'languages)
 
 (defcustom cfengine-indent 2
-  "*Size of a Cfengine indentation step in columns."
+  "*Size of a CFEngine indentation step in columns."
   :group 'cfengine
   :type 'integer)
 
-(defcustom cfengine-mode-abbrevs nil
-  "Abbrevs for Cfengine mode."
+(defcustom cfengine3-parameters-indent '(promise pname 0)
+  "*Indentation of CFEngine3 promise parameters (hanging indent)."
+  :group 'cfengine
+  :type '(list
+          (choice (const :tag "Anchor at beginning of promise" promise)
+                  (const :tag "Anchor at beginning of line" bol))
+          (choice (const :tag "Indent parameter name" pname)
+                  (const :tag "Indent arrow" arrow))
+          (integer :tag "Indentation amount from anchor")))
+
+(defcustom cfengine3-mode-verbose nil
+  "Whether `cfengine-mode' should be verbose
+ (only used by the cfengine3 support currently)"
+  :group 'cfengine
+  :type 'boolean)
+
+(defcustom cfengine2-mode-abbrevs nil
+  "Abbrevs for CFEngine2 mode."
   :group 'cfengine
   :type '(repeat (list (string :tag "Name")
 		       (string :tag "Expansion")
@@ -66,7 +85,7 @@
 
 ;; Taken from the doc for pre-release 2.1.
 (eval-and-compile
-  (defconst cfengine-actions
+  (defconst cfengine2-actions
     '("acl" "alerts" "binservers" "broadcast" "control" "classes" "copy"
       "defaultroute" "disks" "directories" "disable" "editfiles" "files"
       "filters" "groups" "homeservers" "ignore" "import" "interfaces"
@@ -102,7 +121,7 @@
   `(;; Actions.
     ;; List the allowed actions explicitly, so that errors are more obvious.
     (,(concat "^[ \t]*" (eval-when-compile
-			  (regexp-opt cfengine-actions t))
+			  (regexp-opt cfengine2-actions t))
 	      ":")
      1 font-lock-keyword-face)
     ;; Classes.
@@ -117,17 +136,6 @@
 
 (defvar cfengine3-font-lock-keywords
   `(
-    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
-     1 font-lock-keyword-face)
-    (,(concat "^[ \t]*" cfengine3-category-regex)
-     1 font-lock-builtin-face)
-    ;; Variables, including scope, e.g. module.var
-    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
-    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
-    ;; Variable definitions.
-    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
-
-    ;; CFEngine 3.x faces
     ;; defuns
     (,(concat "\\<" cfengine3-defuns-regex "\\>"
               "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
@@ -136,27 +144,43 @@
     (2 font-lock-constant-name-face)
     (3 font-lock-function-name-face)
     (5 font-lock-variable-name-face))
+
+    ;; class selector
+    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
+     1 font-lock-keyword-face)
+
+    ;; category
+    (,(concat "^[ \t]*" cfengine3-category-regex)
+     1 font-lock-builtin-face)
+
+    ;; Variables, including scope, e.g. module.var
+    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
+    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
+
+    ;; Variable definitions.
+    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
+
     ;; variable types
     (,(concat "\\<" (eval-when-compile (regexp-opt cfengine3-vartypes t)) "\\>")
      1 font-lock-type-face)))
 
-(defvar cfengine-imenu-expression
+(defvar cfengine2-imenu-expression
   `((nil ,(concat "^[ \t]*" (eval-when-compile
-			      (regexp-opt cfengine-actions t))
+			      (regexp-opt cfengine2-actions t))
 		  ":[^:]")
 	 1)
     ("Variables/classes" "\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1)
     ("Variables/classes" "\\<define=\\([[:alnum:]_]+\\)" 1)
     ("Variables/classes" "\\<DefineClass\\>[ \t]+\\([[:alnum:]_]+\\)" 1))
-  "`imenu-generic-expression' for Cfengine mode.")
+  "`imenu-generic-expression' for CFEngine mode.")
 
-(defun cfengine-outline-level ()
-  "`outline-level' function for Cfengine mode."
+(defun cfengine2-outline-level ()
+  "`outline-level' function for CFEngine mode."
   (if (looking-at "[^:]+\\(?:[:]+\\)$")
       (length (match-string 1))))
 
-(defun cfengine-beginning-of-defun ()
-  "`beginning-of-defun' function for Cfengine mode.
+(defun cfengine2-beginning-of-defun ()
+  "`beginning-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (unless (<= (current-column) (current-indentation))
     (end-of-line))
@@ -165,8 +189,8 @@
     (goto-char (point-min)))
   t)
 
-(defun cfengine-end-of-defun ()
-  "`end-of-defun' function for Cfengine mode.
+(defun cfengine2-end-of-defun ()
+  "`end-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (end-of-line)
   (if (re-search-forward "^[[:alpha:]]+: *$" nil t)
@@ -176,7 +200,7 @@
 
 ;; Fixme: Should get an extra indent step in editfiles BeginGroup...s.
 
-(defun cfengine-indent-line ()
+(defun cfengine2-indent-line ()
   "Indent a line in Cfengine mode.
 Intended as the value of `indent-line-function'."
   (let ((pos (- (point-max) (point))))
@@ -283,15 +307,17 @@
       (narrow-to-defun)
       (back-to-indentation)
       (setq parse (parse-partial-sexp (point-min) (point)))
-      (message "%S" parse)
+      (when cfengine3-mode-verbose
+        (message "%S" parse))
+
       (cond
-       ;; body/bundle blocks start at 0
+       ;; Body/bundle blocks start at 0.
        ((looking-at (concat cfengine3-defuns-regex "\\>"))
         (indent-line-to 0))
-       ;; categories are indented one step
+       ;; Categories are indented one step.
        ((looking-at (concat cfengine3-category-regex "[ \t]*$"))
         (indent-line-to cfengine-indent))
-       ;; class selectors are indented two steps
+       ;; Class selectors are indented two steps.
        ((looking-at (concat cfengine3-class-selector-regex "[ \t]*$"))
         (indent-line-to (* 2 cfengine-indent)))
        ;; Outdent leading close brackets one step.
@@ -303,13 +329,31 @@
                               (backward-sexp)
                               (current-column)))
           (error nil)))
-       ;; inside a string and it starts before this line
+       ;; Inside a string and it starts before this line.
        ((and (nth 3 parse)
              (< (nth 8 parse) (save-excursion (beginning-of-line) (point))))
         (indent-line-to 0))
-       ;; inside a defun, but not a nested list (depth is 1)
+
+       ;; Inside a defun, but not a nested list (depth is 1).  This is
+       ;; a promise, usually.
        ((= 1 (nth 0 parse))
-        (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))
+        (let ((p-anchor (nth 0 cfengine3-parameters-indent))
+              (p-what (nth 1 cfengine3-parameters-indent))
+              (p-indent (nth 2 cfengine3-parameters-indent)))
+          ;; Do we have the parameter anchor and location and indent
+          ;; defined, and are we looking at a promise parameter?
+          (if (and p-anchor p-what p-indent
+                   (looking-at  "\\([[:alnum:]_]+[ \t]*\\)=>"))
+              (let* ((arrow-offset (* -1 (length (match-string 1))))
+                     (extra-offset (if (eq p-what 'arrow) arrow-offset 0))
+                     (base-offset (if (eq p-anchor 'promise)
+                                      (* (+ 2 (nth 0 parse)) cfengine-indent)
+                                    0)))
+                (indent-line-to (max 0 (+ p-indent base-offset extra-offset))))
+            ;; Else, indent to cfengine-indent times the nested depth
+            ;; plus 2.  That way, promises indent deeper than class
+            ;; selectors, which in turn are one deeper than categories.
+          (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))))
        ;; Inside brackets/parens: indent to start column of non-comment
        ;; token on line following open bracket or by one step from open
        ;; bracket's column.
@@ -421,8 +465,8 @@
   (modify-syntax-entry ?\\ "." table))
 
 ;;;###autoload
-(define-derived-mode cfengine3-mode prog-mode "CFEngine3"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine3-mode prog-mode "CFE3"
+  "Major mode for editing CFEngine3 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
@@ -441,39 +485,39 @@
        #'cfengine3-end-of-defun))
 
 ;;;###autoload
-(define-derived-mode cfengine-mode prog-mode "Cfengine"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine2-mode prog-mode "CFE2"
+  "Major mode for editing CFEngine2 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
 to the action header."
   (cfengine-common-settings)
-  (cfengine-common-syntax cfengine-mode-syntax-table)
+  (cfengine-common-syntax cfengine2-mode-syntax-table)
 
   ;; Shell commands can be quoted by single, double or back quotes.
   ;; It's debatable whether we should define string syntax, but it
   ;; should avoid potential confusion in some cases.
-  (modify-syntax-entry ?\' "\"" cfengine-mode-syntax-table)
-  (modify-syntax-entry ?\` "\"" cfengine-mode-syntax-table)
+  (modify-syntax-entry ?\' "\"" cfengine2-mode-syntax-table)
+  (modify-syntax-entry ?\` "\"" cfengine2-mode-syntax-table)
 
-  (set (make-local-variable 'indent-line-function) #'cfengine-indent-line)
+  (set (make-local-variable 'indent-line-function) #'cfengine2-indent-line)
   (set (make-local-variable 'outline-regexp) "[ \t]*\\(\\sw\\|\\s_\\)+:+")
-  (set (make-local-variable 'outline-level) #'cfengine-outline-level)
+  (set (make-local-variable 'outline-level) #'cfengine2-outline-level)
   (set (make-local-variable 'fill-paragraph-function)
        #'cfengine-fill-paragraph)
-  (define-abbrev-table 'cfengine-mode-abbrev-table cfengine-mode-abbrevs)
+  (define-abbrev-table 'cfengine2-mode-abbrev-table cfengine2-mode-abbrevs)
   (setq font-lock-defaults
-	'(cfengine-font-lock-keywords nil nil nil beginning-of-line))
+        '(cfengine-font-lock-keywords nil nil nil beginning-of-line))
   ;; Fixme: set the args of functions in evaluated classes to string
   ;; syntax, and then obey syntax properties.
-  (setq imenu-generic-expression cfengine-imenu-expression)
+  (setq imenu-generic-expression cfengine2-imenu-expression)
   (set (make-local-variable 'beginning-of-defun-function)
-       #'cfengine-beginning-of-defun)
-  (set (make-local-variable 'end-of-defun-function) #'cfengine-end-of-defun))
+       #'cfengine2-beginning-of-defun)
+  (set (make-local-variable 'end-of-defun-function) #'cfengine2-end-of-defun))
 
 ;;;###autoload
 (defun cfengine-auto-mode ()
-  "Choose between `cfengine-mode' and `cfengine3-mode' depending
+  "Choose between `cfengine2-mode' and `cfengine3-mode' depending
 on the buffer contents"
   (let ((v3 nil))
     (save-restriction
@@ -481,9 +525,12 @@
       (while (not (or (eobp) v3))
         (setq v3 (looking-at (concat cfengine3-defuns-regex "\\>")))
         (forward-line)))
-    (if v3 (cfengine3-mode) (cfengine-mode))))
+    (if v3 (cfengine3-mode) (cfengine2-mode))))
+
+(defalias 'cfengine-mode 'cfengine-auto-mode)
 
 (provide 'cfengine3)
+(provide 'cfengine2)
 (provide 'cfengine)
 
 ;;; cfengine.el ends here

Re: changes to cfengine-mode

[Stefan Monnier]

>>> I'd like to rename `cfengine-mode' to `cfengine2-mode', and
>>> `cfengine-auto-mode' to `cfengine-mode' (`cfengine-auto-mode' loads
>>> `cfengine3-mode' if it thinks a cfengine v.3 file is edited).
SM> That sounds right.
> Thanks for looking.  Attached is a patch that will:
> 1) rename `cfengine-mode' to `cfengine2-mode' and defalias
> 'cfengine-mode to 'cfengine-auto-mode.  Provide 'cfengine2.

That's good.

> 2) rename all the "cfengine-*" variables used only by `cfengine2-mode'
> to "cfengine2-*"

Is this needed?  Do these variables only make sense for cfengine2 syntax?

> 3) adjust the commentary to match

Good.

> 4) give a version and set it to 1.1 for this release

We don't like versions for files bundled with Emacs (only serves to
cause spurious merge conflicts), so if you can keep this outside of the
tree, it's preferable.

> 5) use "CFEngine" consistently, capitalized as the author, Mark Burgess,
> does.

Good.

> 6) add `cfengine3-mode-verbose' to control some debugging info

Why not `cfengine-mode-verbose'?  Also, if it's for debugging, why
call it "verbose" rather than "debug"?

> 7) add `cfengine3-parameters-indent' to address some shortcomings in
> indentation.  This is a new feature but necessary to fix the lack of a
> hanging indent in the current `cfengine3-mode'.  It's very low-risk and
> I hope it's acceptable.

I'd keep it with a "cfengine-" prefix.  Basically, I'd keep
user-facing names with a "cfengine-" prefix whenever possible.

> 8) improve and capitalize comments in many places, though that work is
> not complete

Good.

> 9) change the "lighter" modeline indicators from "Cfengine[23]" to
> "CFE[23]" to take less space

No opinion on this.

> I also have a font-lock issue I can't figure out with the current
> version.  Given the following code (use `cfengine3-mode' to see the
> problem):

*Messages* tells me:
Error during redisplay: (void-variable font-lock-constant-name-face)

Some comments on the code follow.


        Stefan


> +(defvar cfengine-version nil)
> +(setq cfengine-version "1.1")

Regardless of whether we keep this, the above is atrocious.  Use `defconst'.

> +(defcustom cfengine3-parameters-indent '(promise pname 0)
> +  "*Indentation of CFEngine3 promise parameters (hanging indent)."

This description is incomprehensible to me.  It should describe the
accepted values and their meaning.  Also better include a short example
to make it clear what this controls.

> +(defcustom cfengine2-mode-abbrevs nil
> +  "Abbrevs for CFEngine2 mode."
>    :group 'cfengine
>    :type '(repeat (list (string :tag "Name")
>  		       (string :tag "Expansion")

You haven't added the corresponding feature for cfengine3, and I think
I agree with this choice, but the reason is not that it made sense for
cfengine2 and it doesn't for cfengine3, but simply that there's no need
for a config var for it.  There's already `edit-abbrevs'.
So rather than rename it, I'd obsolete it and mention that it only
applies to cfengine2-mode.

> -  (defconst cfengine-actions
> +  (defconst cfengine2-actions

I think cfengine-font-lock-keywords also needs to be renamed to
cfengine2 prefix.

>  (defvar cfengine3-font-lock-keywords
>    `(
> -    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
> -     1 font-lock-keyword-face)
> -    (,(concat "^[ \t]*" cfengine3-category-regex)
> -     1 font-lock-builtin-face)
> -    ;; Variables, including scope, e.g. module.var
> -    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
> -    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
> -    ;; Variable definitions.
> -    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
> -
> -    ;; CFEngine 3.x faces
>      ;; defuns
>      (,(concat "\\<" cfengine3-defuns-regex "\\>"
>                "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
> @@ -136,27 +144,43 @@
>      (2 font-lock-constant-name-face)
>      (3 font-lock-function-name-face)
>      (5 font-lock-variable-name-face))
> +
> +    ;; class selector
> +    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
> +     1 font-lock-keyword-face)
> +
> +    ;; category
> +    (,(concat "^[ \t]*" cfengine3-category-regex)
> +     1 font-lock-builtin-face)
> +
> +    ;; Variables, including scope, e.g. module.var
> +    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
> +    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
> +
> +    ;; Variable definitions.
> +    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
> +
>      ;; variable types
>      (,(concat "\\<" (eval-when-compile (regexp-opt cfengine3-vartypes t)) "\\>")
>       1 font-lock-type-face)))

Not sure what this is about.  You'd need to describe it in
the ChangeLog.  The comments should be capitalized.

> +(provide 'cfengine2)

Don't bother.


        Stefan

Re: changes to cfengine-mode

[Stefan Monnier]

> 7) add `cfengine3-parameters-indent' to address some shortcomings in
> indentation.  This is a new feature but necessary to fix the lack of a
> hanging indent in the current `cfengine3-mode'.  It's very low-risk and
> I hope it's acceptable.

BTW, I understand you think it's low-risk, but this will have to wait
for 24.2, unless you can come up with a very compelling detailed
argument for why it's both low-risk and high-importance.


        Stefan

Re: changes to cfengine-mode

[Chong Yidong]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> 4) give a version and set it to 1.1 for this release
>
> We don't like versions for files bundled with Emacs (only serves to
> cause spurious merge conflicts), so if you can keep this outside of the
> tree, it's preferable.

Surely such merge conflicts are trivial to resolve.  Having version
numbers has the advantage that it should DTRT with ELPA if you have both
a built-in package and an external package.

Re: changes to cfengine-mode

[Ted Zlatanov]

On Wed, 07 Dec 2011 11:12:04 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote: 

>> 7) add `cfengine3-parameters-indent' to address some shortcomings in
>> indentation.  This is a new feature but necessary to fix the lack of a
>> hanging indent in the current `cfengine3-mode'.  It's very low-risk and
>> I hope it's acceptable.

SM> BTW, I understand you think it's low-risk, but this will have to wait
SM> for 24.2, unless you can come up with a very compelling detailed
SM> argument for why it's both low-risk and high-importance.

It can wait.

Ted

Re: changes to cfengine-mode

[Ted Zlatanov]

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

On Wed, 07 Dec 2011 11:10:23 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote: 

>> 2) rename all the "cfengine-*" variables used only by `cfengine2-mode'
>> to "cfengine2-*"

SM> Is this needed?  Do these variables only make sense for cfengine2 syntax?

Yes, I renamed those that are only for cfengine2.

>> 4) give a version and set it to 1.1 for this release

SM> We don't like versions for files bundled with Emacs (only serves to
SM> cause spurious merge conflicts), so if you can keep this outside of the
SM> tree, it's preferable.

I'm OK either way. The attached patch still has it as per Chong's
comment.

>> 6) add `cfengine3-mode-verbose' to control some debugging info

SM> Why not `cfengine-mode-verbose'?  Also, if it's for debugging, why
SM> call it "verbose" rather than "debug"?

Fixed.

>> 7) add `cfengine3-parameters-indent' to address some shortcomings in
>> indentation.  This is a new feature but necessary to fix the lack of a
>> hanging indent in the current `cfengine3-mode'.  It's very low-risk and
>> I hope it's acceptable.

SM> I'd keep it with a "cfengine-" prefix.  Basically, I'd keep
SM> user-facing names with a "cfengine-" prefix whenever possible.

Fixed (but it will stay out until 24.2).

>> +(defvar cfengine-version nil)
>> +(setq cfengine-version "1.1")

SM> Regardless of whether we keep this, the above is atrocious.  Use `defconst'.

Removed.

>> +(defcustom cfengine3-parameters-indent '(promise pname 0)
>> +  "*Indentation of CFEngine3 promise parameters (hanging indent)."

SM> This description is incomprehensible to me.  It should describe the
SM> accepted values and their meaning.  Also better include a short example
SM> to make it clear what this controls.

Fixed.

>> +(defcustom cfengine2-mode-abbrevs nil
>> +  "Abbrevs for CFEngine2 mode."
>> :group 'cfengine
>> :type '(repeat (list (string :tag "Name")
>> (string :tag "Expansion")

SM> You haven't added the corresponding feature for cfengine3, and I think
SM> I agree with this choice, but the reason is not that it made sense for
SM> cfengine2 and it doesn't for cfengine3, but simply that there's no need
SM> for a config var for it.  There's already `edit-abbrevs'.
SM> So rather than rename it, I'd obsolete it and mention that it only
SM> applies to cfengine2-mode.

OK; done.

>> -  (defconst cfengine-actions
>> +  (defconst cfengine2-actions

SM> I think cfengine-font-lock-keywords also needs to be renamed to
SM> cfengine2 prefix.

OK.

>> (defvar cfengine3-font-lock-keywords
...
SM> Not sure what this is about.  You'd need to describe it in
SM> the ChangeLog.  The comments should be capitalized.

I'm not sure what you mean?  It's the font-lock keywords?

>> +(provide 'cfengine2)

SM> Don't bother.

OK.

I'll commit the revised code after 24.1 is out.  There's no need for it
sooner, unless you want it.  In that case I'll remove
`cfengine-parameter-indent'.

Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cfengine-mode-revised.patch --]
[-- Type: text/x-diff, Size: 14042 bytes --]

=== modified file 'lisp/progmodes/cfengine.el'
--- lisp/progmodes/cfengine.el	2011-11-20 03:48:53 +0000
+++ lisp/progmodes/cfengine.el	2011-12-13 17:09:20 +0000
@@ -5,6 +5,7 @@
 ;; Author: Dave Love <fx@gnu.org>
 ;; Maintainer: Ted Zlatanov <tzz@lifelogs.com>
 ;; Keywords: languages
+;; Version: 1.1
 
 ;; This file is part of GNU Emacs.
 
@@ -29,18 +30,18 @@
 ;; The CFEngine 3.x support doesn't have Imenu support but patches are
 ;; welcome.
 
-;; You can set it up so either cfengine-mode (2.x and earlier) or
-;; cfengine3-mode (3.x) will be picked, depending on the buffer
+;; You can set it up so either `cfengine2-mode' (2.x and earlier) or
+;; `cfengine3-mode' (3.x) will be picked, depending on the buffer
 ;; contents:
 
-;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-auto-mode))
+;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-mode))
 
 ;; OR you can choose to always use a specific version, if you prefer
-;; it
+;; it:
 
 ;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine3-mode))
-;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine-mode))
-;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine-mode))
+;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine2-mode))
+;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine2-mode))
 
 ;; This is not the same as the mode written by Rolf Ebert
 ;; <ebert@waporo.muc.de>, distributed with cfengine-2.0.5.  It does
@@ -49,24 +50,96 @@
 ;;; Code:
 
 (defgroup cfengine ()
-  "Editing Cfengine files."
+  "Editing CFEngine files."
   :group 'languages)
 
 (defcustom cfengine-indent 2
-  "*Size of a Cfengine indentation step in columns."
+  "*Size of a CFEngine indentation step in columns."
   :group 'cfengine
   :type 'integer)
 
+(defcustom cfengine-parameters-indent '(promise pname 0)
+  "*Indentation of CFEngine3 promise parameters (hanging indent).
+
+For example, say you have this code:
+
+bundle x y
+{
+  section:
+    class::
+      promise ...
+      promiseparameter => ...
+}
+
+You can choose to indent promiseparameter from the beginning of
+the line (absolutely) or from the word \"promise\" (relatively).
+
+You can also choose to indent the start of the word
+\"promiseparameter\" or the arrow that follows it.
+
+Finally, you can choose the amount of the indent.
+
+The default is to anchor at promise, indent parameter name, and offset 0:
+
+bundle agent rcfiles
+{
+  files:
+    any::
+      \"/tmp/netrc\"
+      comment => \"my netrc\",
+      perms => mog(\"600\", \"tzz\", \"tzz\");
+}
+
+Here we anchor at beginning of line, indent arrow, and offset 10:
+
+bundle agent rcfiles
+{
+  files:
+    any::
+      \"/tmp/netrc\"
+  comment => \"my netrc\",
+    perms => mog(\"600\", \"tzz\", \"tzz\");
+}
+
+Some, including cfengine_stdlib.cf, like to anchor at promise, indent
+arrow, and offset 16 or so:
+
+bundle agent rcfiles
+{
+  files:
+    any::
+      \"/tmp/netrc\"
+              comment => \"my netrc\",
+                perms => mog(\"600\", \"tzz\", \"tzz\");
+}
+"
+  :group 'cfengine
+  :type '(list
+          (choice (const :tag "Anchor at beginning of promise" promise)
+                  (const :tag "Anchor at beginning of line" bol))
+          (choice (const :tag "Indent parameter name" pname)
+                  (const :tag "Indent arrow" arrow))
+          (integer :tag "Indentation amount from anchor")))
+
+(defcustom cfengine-mode-debug nil
+  "Whether `cfengine-mode' should print debugging info
+ (only used by the cfengine3 support currently)"
+  :group 'cfengine
+  :type 'boolean)
+
 (defcustom cfengine-mode-abbrevs nil
-  "Abbrevs for Cfengine mode."
+  "Abbrevs for CFEngine2 mode.
+Obsolete, see `edit-abbrevs' instead."
   :group 'cfengine
   :type '(repeat (list (string :tag "Name")
 		       (string :tag "Expansion")
 		       (choice  :tag "Hook" (const nil) function))))
 
+(make-obsolete-variable 'cfengine-mode-abbrevs nil "24.1")
+
 ;; Taken from the doc for pre-release 2.1.
 (eval-and-compile
-  (defconst cfengine-actions
+  (defconst cfengine2-actions
     '("acl" "alerts" "binservers" "broadcast" "control" "classes" "copy"
       "defaultroute" "disks" "directories" "disable" "editfiles" "files"
       "filters" "groups" "homeservers" "ignore" "import" "interfaces"
@@ -98,11 +171,11 @@
      '(string int real slist ilist rlist irange rrange counter))
     "List of the CFEngine 3.x variable types."))
 
-(defvar cfengine-font-lock-keywords
+(defvar cfengine2-font-lock-keywords
   `(;; Actions.
     ;; List the allowed actions explicitly, so that errors are more obvious.
     (,(concat "^[ \t]*" (eval-when-compile
-			  (regexp-opt cfengine-actions t))
+			  (regexp-opt cfengine2-actions t))
 	      ":")
      1 font-lock-keyword-face)
     ;; Classes.
@@ -117,17 +190,6 @@
 
 (defvar cfengine3-font-lock-keywords
   `(
-    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
-     1 font-lock-keyword-face)
-    (,(concat "^[ \t]*" cfengine3-category-regex)
-     1 font-lock-builtin-face)
-    ;; Variables, including scope, e.g. module.var
-    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
-    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
-    ;; Variable definitions.
-    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
-
-    ;; CFEngine 3.x faces
     ;; defuns
     (,(concat "\\<" cfengine3-defuns-regex "\\>"
               "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
@@ -136,27 +198,43 @@
     (2 font-lock-constant-name-face)
     (3 font-lock-function-name-face)
     (5 font-lock-variable-name-face))
+
+    ;; class selector
+    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
+     1 font-lock-keyword-face)
+
+    ;; category
+    (,(concat "^[ \t]*" cfengine3-category-regex)
+     1 font-lock-builtin-face)
+
+    ;; Variables, including scope, e.g. module.var
+    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
+    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
+
+    ;; Variable definitions.
+    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
+
     ;; variable types
     (,(concat "\\<" (eval-when-compile (regexp-opt cfengine3-vartypes t)) "\\>")
      1 font-lock-type-face)))
 
-(defvar cfengine-imenu-expression
+(defvar cfengine2-imenu-expression
   `((nil ,(concat "^[ \t]*" (eval-when-compile
-			      (regexp-opt cfengine-actions t))
+			      (regexp-opt cfengine2-actions t))
 		  ":[^:]")
 	 1)
     ("Variables/classes" "\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1)
     ("Variables/classes" "\\<define=\\([[:alnum:]_]+\\)" 1)
     ("Variables/classes" "\\<DefineClass\\>[ \t]+\\([[:alnum:]_]+\\)" 1))
-  "`imenu-generic-expression' for Cfengine mode.")
+  "`imenu-generic-expression' for CFEngine mode.")
 
-(defun cfengine-outline-level ()
-  "`outline-level' function for Cfengine mode."
+(defun cfengine2-outline-level ()
+  "`outline-level' function for CFEngine mode."
   (if (looking-at "[^:]+\\(?:[:]+\\)$")
       (length (match-string 1))))
 
-(defun cfengine-beginning-of-defun ()
-  "`beginning-of-defun' function for Cfengine mode.
+(defun cfengine2-beginning-of-defun ()
+  "`beginning-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (unless (<= (current-column) (current-indentation))
     (end-of-line))
@@ -165,8 +243,8 @@
     (goto-char (point-min)))
   t)
 
-(defun cfengine-end-of-defun ()
-  "`end-of-defun' function for Cfengine mode.
+(defun cfengine2-end-of-defun ()
+  "`end-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (end-of-line)
   (if (re-search-forward "^[[:alpha:]]+: *$" nil t)
@@ -176,7 +254,7 @@
 
 ;; Fixme: Should get an extra indent step in editfiles BeginGroup...s.
 
-(defun cfengine-indent-line ()
+(defun cfengine2-indent-line ()
   "Indent a line in Cfengine mode.
 Intended as the value of `indent-line-function'."
   (let ((pos (- (point-max) (point))))
@@ -283,15 +361,17 @@
       (narrow-to-defun)
       (back-to-indentation)
       (setq parse (parse-partial-sexp (point-min) (point)))
-      (message "%S" parse)
+      (when cfengine-mode-debug
+        (message "%S" parse))
+
       (cond
-       ;; body/bundle blocks start at 0
+       ;; Body/bundle blocks start at 0.
        ((looking-at (concat cfengine3-defuns-regex "\\>"))
         (indent-line-to 0))
-       ;; categories are indented one step
+       ;; Categories are indented one step.
        ((looking-at (concat cfengine3-category-regex "[ \t]*$"))
         (indent-line-to cfengine-indent))
-       ;; class selectors are indented two steps
+       ;; Class selectors are indented two steps.
        ((looking-at (concat cfengine3-class-selector-regex "[ \t]*$"))
         (indent-line-to (* 2 cfengine-indent)))
        ;; Outdent leading close brackets one step.
@@ -303,13 +383,31 @@
                               (backward-sexp)
                               (current-column)))
           (error nil)))
-       ;; inside a string and it starts before this line
+       ;; Inside a string and it starts before this line.
        ((and (nth 3 parse)
              (< (nth 8 parse) (save-excursion (beginning-of-line) (point))))
         (indent-line-to 0))
-       ;; inside a defun, but not a nested list (depth is 1)
+
+       ;; Inside a defun, but not a nested list (depth is 1).  This is
+       ;; a promise, usually.
        ((= 1 (nth 0 parse))
-        (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))
+        (let ((p-anchor (nth 0 cfengine-parameters-indent))
+              (p-what (nth 1 cfengine-parameters-indent))
+              (p-indent (nth 2 cfengine-parameters-indent)))
+          ;; Do we have the parameter anchor and location and indent
+          ;; defined, and are we looking at a promise parameter?
+          (if (and p-anchor p-what p-indent
+                   (looking-at  "\\([[:alnum:]_]+[ \t]*\\)=>"))
+              (let* ((arrow-offset (* -1 (length (match-string 1))))
+                     (extra-offset (if (eq p-what 'arrow) arrow-offset 0))
+                     (base-offset (if (eq p-anchor 'promise)
+                                      (* (+ 2 (nth 0 parse)) cfengine-indent)
+                                    0)))
+                (indent-line-to (max 0 (+ p-indent base-offset extra-offset))))
+            ;; Else, indent to cfengine-indent times the nested depth
+            ;; plus 2.  That way, promises indent deeper than class
+            ;; selectors, which in turn are one deeper than categories.
+          (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))))
        ;; Inside brackets/parens: indent to start column of non-comment
        ;; token on line following open bracket or by one step from open
        ;; bracket's column.
@@ -421,8 +519,8 @@
   (modify-syntax-entry ?\\ "." table))
 
 ;;;###autoload
-(define-derived-mode cfengine3-mode prog-mode "CFEngine3"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine3-mode prog-mode "CFE3"
+  "Major mode for editing CFEngine3 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
@@ -441,39 +539,39 @@
        #'cfengine3-end-of-defun))
 
 ;;;###autoload
-(define-derived-mode cfengine-mode prog-mode "Cfengine"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine2-mode prog-mode "CFE2"
+  "Major mode for editing CFEngine2 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
 to the action header."
   (cfengine-common-settings)
-  (cfengine-common-syntax cfengine-mode-syntax-table)
+  (cfengine-common-syntax cfengine2-mode-syntax-table)
 
   ;; Shell commands can be quoted by single, double or back quotes.
   ;; It's debatable whether we should define string syntax, but it
   ;; should avoid potential confusion in some cases.
-  (modify-syntax-entry ?\' "\"" cfengine-mode-syntax-table)
-  (modify-syntax-entry ?\` "\"" cfengine-mode-syntax-table)
+  (modify-syntax-entry ?\' "\"" cfengine2-mode-syntax-table)
+  (modify-syntax-entry ?\` "\"" cfengine2-mode-syntax-table)
 
-  (set (make-local-variable 'indent-line-function) #'cfengine-indent-line)
+  (set (make-local-variable 'indent-line-function) #'cfengine2-indent-line)
   (set (make-local-variable 'outline-regexp) "[ \t]*\\(\\sw\\|\\s_\\)+:+")
-  (set (make-local-variable 'outline-level) #'cfengine-outline-level)
+  (set (make-local-variable 'outline-level) #'cfengine2-outline-level)
   (set (make-local-variable 'fill-paragraph-function)
        #'cfengine-fill-paragraph)
-  (define-abbrev-table 'cfengine-mode-abbrev-table cfengine-mode-abbrevs)
+  (define-abbrev-table 'cfengine2-mode-abbrev-table cfengine-mode-abbrevs)
   (setq font-lock-defaults
-	'(cfengine-font-lock-keywords nil nil nil beginning-of-line))
+        '(cfengine2-font-lock-keywords nil nil nil beginning-of-line))
   ;; Fixme: set the args of functions in evaluated classes to string
   ;; syntax, and then obey syntax properties.
-  (setq imenu-generic-expression cfengine-imenu-expression)
+  (setq imenu-generic-expression cfengine2-imenu-expression)
   (set (make-local-variable 'beginning-of-defun-function)
-       #'cfengine-beginning-of-defun)
-  (set (make-local-variable 'end-of-defun-function) #'cfengine-end-of-defun))
+       #'cfengine2-beginning-of-defun)
+  (set (make-local-variable 'end-of-defun-function) #'cfengine2-end-of-defun))
 
 ;;;###autoload
 (defun cfengine-auto-mode ()
-  "Choose between `cfengine-mode' and `cfengine3-mode' depending
+  "Choose between `cfengine2-mode' and `cfengine3-mode' depending
 on the buffer contents"
   (let ((v3 nil))
     (save-restriction
@@ -481,7 +579,9 @@
       (while (not (or (eobp) v3))
         (setq v3 (looking-at (concat cfengine3-defuns-regex "\\>")))
         (forward-line)))
-    (if v3 (cfengine3-mode) (cfengine-mode))))
+    (if v3 (cfengine3-mode) (cfengine2-mode))))
+
+(defalias 'cfengine-mode 'cfengine-auto-mode)
 
 (provide 'cfengine3)
 (provide 'cfengine)

Re: changes to cfengine-mode

[Ted Zlatanov]

On Wed, 07 Dec 2011 11:10:23 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote: 

>> I also have a font-lock issue I can't figure out with the current
>> version.  Given the following code (use `cfengine3-mode' to see the
>> problem):

SM> *Messages* tells me:
SM> Error during redisplay: (void-variable font-lock-constant-name-face)

I don't know why it says that.  That variable is defined in
font-lock.el, right?  Or am I missing something?

Ted

Re: changes to cfengine-mode

[Stefan Monnier]

>>> 2) rename all the "cfengine-*" variables used only by `cfengine2-mode'
>>> to "cfengine2-*"
SM> Is this needed?  Do these variables only make sense for cfengine2 syntax?
> Yes, I renamed those that are only for cfengine2.

There's a difference between "only used for cfengine2" and "only makes
sense for cfengine2".  E.g. it might only be used for cfengine2 because
it's just not implemented for cfengine3 (e.g. for lack of resources, or
because some other generic Emacs feature can be used instead).

Basically, renaming user variables is a hassle, so it should only be done
when it's really needed, e.g. to avoid conflicts or fix a real problem.

> I'll commit the revised code after 24.1 is out.

I'd like to see the shuffle (cfengine-mode->cfengine2-mode and
cfengine-auto-mode->cfengine-mode) be in Emacs-24.1.  You can include
the docfixes at the same time.

SM> *Messages* tells me:
SM> Error during redisplay: (void-variable font-lock-constant-name-face)
> I don't know why it says that.  That variable is defined in
> font-lock.el, right?  Or am I missing something?

Yes, you're missing the fact that it's not defined ;-)
BTW, to debug font-lock problem, I recommend you start by (setq
font-lock-support-mode nil), so that debug-on-error works.


        Stefan

Re: changes to cfengine-mode

[Ted Zlatanov]

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

On Tue, 13 Dec 2011 14:51:48 -0500 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> I'll commit the revised code after 24.1 is out.

SM> I'd like to see the shuffle (cfengine-mode->cfengine2-mode and
SM> cfengine-auto-mode-> cfengine-mode) be in Emacs-24.1.  You can include
SM> the docfixes at the same time.

OK.  See attached, and I hope final, patch.

SM> *Messages* tells me:
SM> Error during redisplay: (void-variable font-lock-constant-name-face)
>> I don't know why it says that.  That variable is defined in
>> font-lock.el, right?  Or am I missing something?

SM> Yes, you're missing the fact that it's not defined ;-)
SM> BTW, to debug font-lock problem, I recommend you start by (setq
SM> font-lock-support-mode nil), so that debug-on-error works.

Bah.  I don't know how I missed than it's actually
`font-lock-constant-face'.  Fixed.

Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cfengine-mode-revised2.patch --]
[-- Type: text/x-diff, Size: 11567 bytes --]

=== modified file 'lisp/progmodes/cfengine.el'
--- lisp/progmodes/cfengine.el	2011-11-20 03:48:53 +0000
+++ lisp/progmodes/cfengine.el	2011-12-13 21:30:46 +0000
@@ -5,6 +5,7 @@
 ;; Author: Dave Love <fx@gnu.org>
 ;; Maintainer: Ted Zlatanov <tzz@lifelogs.com>
 ;; Keywords: languages
+;; Version: 1.1
 
 ;; This file is part of GNU Emacs.
 
@@ -29,18 +30,18 @@
 ;; The CFEngine 3.x support doesn't have Imenu support but patches are
 ;; welcome.
 
-;; You can set it up so either cfengine-mode (2.x and earlier) or
-;; cfengine3-mode (3.x) will be picked, depending on the buffer
+;; You can set it up so either `cfengine2-mode' (2.x and earlier) or
+;; `cfengine3-mode' (3.x) will be picked, depending on the buffer
 ;; contents:
 
-;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-auto-mode))
+;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-mode))
 
 ;; OR you can choose to always use a specific version, if you prefer
-;; it
+;; it:
 
 ;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine3-mode))
-;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine-mode))
-;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine-mode))
+;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine2-mode))
+;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine2-mode))
 
 ;; This is not the same as the mode written by Rolf Ebert
 ;; <ebert@waporo.muc.de>, distributed with cfengine-2.0.5.  It does
@@ -49,24 +50,33 @@
 ;;; Code:
 
 (defgroup cfengine ()
-  "Editing Cfengine files."
+  "Editing CFEngine files."
   :group 'languages)
 
 (defcustom cfengine-indent 2
-  "*Size of a Cfengine indentation step in columns."
+  "*Size of a CFEngine indentation step in columns."
   :group 'cfengine
   :type 'integer)
 
+(defcustom cfengine-mode-debug nil
+  "Whether `cfengine-mode' should print debugging info
+ (only used by the cfengine3 support currently)"
+  :group 'cfengine
+  :type 'boolean)
+
 (defcustom cfengine-mode-abbrevs nil
-  "Abbrevs for Cfengine mode."
+  "Abbrevs for CFEngine2 mode.
+Obsolete, see `edit-abbrevs' instead."
   :group 'cfengine
   :type '(repeat (list (string :tag "Name")
 		       (string :tag "Expansion")
 		       (choice  :tag "Hook" (const nil) function))))
 
+(make-obsolete-variable 'cfengine-mode-abbrevs nil "24.1")
+
 ;; Taken from the doc for pre-release 2.1.
 (eval-and-compile
-  (defconst cfengine-actions
+  (defconst cfengine2-actions
     '("acl" "alerts" "binservers" "broadcast" "control" "classes" "copy"
       "defaultroute" "disks" "directories" "disable" "editfiles" "files"
       "filters" "groups" "homeservers" "ignore" "import" "interfaces"
@@ -98,11 +108,11 @@
      '(string int real slist ilist rlist irange rrange counter))
     "List of the CFEngine 3.x variable types."))
 
-(defvar cfengine-font-lock-keywords
+(defvar cfengine2-font-lock-keywords
   `(;; Actions.
     ;; List the allowed actions explicitly, so that errors are more obvious.
     (,(concat "^[ \t]*" (eval-when-compile
-			  (regexp-opt cfengine-actions t))
+			  (regexp-opt cfengine2-actions t))
 	      ":")
      1 font-lock-keyword-face)
     ;; Classes.
@@ -117,46 +127,58 @@
 
 (defvar cfengine3-font-lock-keywords
   `(
+    ;; defuns
+    (,(concat "\\<" cfengine3-defuns-regex "\\>"
+              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
+              "[ \t]+\\<\\([[:alnum:]_]+\\)\\((\\([^)]*\\))\\)")
+    (1 font-lock-builtin-face)
+    (2 font-lock-constant-face)
+    (3 font-lock-function-name-face)
+    (5 font-lock-variable-name-face))
+
+    (,(concat "\\<" cfengine3-defuns-regex "\\>"
+              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
+              "[ \t]+\\<\\([[:alnum:]_]+\\)")
+    (1 font-lock-builtin-face)
+    (2 font-lock-constant-face)
+    (3 font-lock-function-name-face))
+
+    ;; class selector
     (,(concat "^[ \t]*" cfengine3-class-selector-regex)
      1 font-lock-keyword-face)
+
+    ;; category
     (,(concat "^[ \t]*" cfengine3-category-regex)
      1 font-lock-builtin-face)
+
     ;; Variables, including scope, e.g. module.var
     ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
     ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
+
     ;; Variable definitions.
     ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
 
-    ;; CFEngine 3.x faces
-    ;; defuns
-    (,(concat "\\<" cfengine3-defuns-regex "\\>"
-              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
-              "[ \t]+\\<\\([[:alnum:]_]+\\)\\((\\([^)]*\\))\\)?")
-    (1 font-lock-builtin-face)
-    (2 font-lock-constant-name-face)
-    (3 font-lock-function-name-face)
-    (5 font-lock-variable-name-face))
     ;; variable types
     (,(concat "\\<" (eval-when-compile (regexp-opt cfengine3-vartypes t)) "\\>")
      1 font-lock-type-face)))
 
-(defvar cfengine-imenu-expression
+(defvar cfengine2-imenu-expression
   `((nil ,(concat "^[ \t]*" (eval-when-compile
-			      (regexp-opt cfengine-actions t))
+			      (regexp-opt cfengine2-actions t))
 		  ":[^:]")
 	 1)
     ("Variables/classes" "\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1)
     ("Variables/classes" "\\<define=\\([[:alnum:]_]+\\)" 1)
     ("Variables/classes" "\\<DefineClass\\>[ \t]+\\([[:alnum:]_]+\\)" 1))
-  "`imenu-generic-expression' for Cfengine mode.")
+  "`imenu-generic-expression' for CFEngine mode.")
 
-(defun cfengine-outline-level ()
-  "`outline-level' function for Cfengine mode."
+(defun cfengine2-outline-level ()
+  "`outline-level' function for CFEngine mode."
   (if (looking-at "[^:]+\\(?:[:]+\\)$")
       (length (match-string 1))))
 
-(defun cfengine-beginning-of-defun ()
-  "`beginning-of-defun' function for Cfengine mode.
+(defun cfengine2-beginning-of-defun ()
+  "`beginning-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (unless (<= (current-column) (current-indentation))
     (end-of-line))
@@ -165,8 +187,8 @@
     (goto-char (point-min)))
   t)
 
-(defun cfengine-end-of-defun ()
-  "`end-of-defun' function for Cfengine mode.
+(defun cfengine2-end-of-defun ()
+  "`end-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (end-of-line)
   (if (re-search-forward "^[[:alpha:]]+: *$" nil t)
@@ -176,7 +198,7 @@
 
 ;; Fixme: Should get an extra indent step in editfiles BeginGroup...s.
 
-(defun cfengine-indent-line ()
+(defun cfengine2-indent-line ()
   "Indent a line in Cfengine mode.
 Intended as the value of `indent-line-function'."
   (let ((pos (- (point-max) (point))))
@@ -283,15 +305,17 @@
       (narrow-to-defun)
       (back-to-indentation)
       (setq parse (parse-partial-sexp (point-min) (point)))
-      (message "%S" parse)
+      (when cfengine-mode-debug
+        (message "%S" parse))
+
       (cond
-       ;; body/bundle blocks start at 0
+       ;; Body/bundle blocks start at 0.
        ((looking-at (concat cfengine3-defuns-regex "\\>"))
         (indent-line-to 0))
-       ;; categories are indented one step
+       ;; Categories are indented one step.
        ((looking-at (concat cfengine3-category-regex "[ \t]*$"))
         (indent-line-to cfengine-indent))
-       ;; class selectors are indented two steps
+       ;; Class selectors are indented two steps.
        ((looking-at (concat cfengine3-class-selector-regex "[ \t]*$"))
         (indent-line-to (* 2 cfengine-indent)))
        ;; Outdent leading close brackets one step.
@@ -303,11 +327,17 @@
                               (backward-sexp)
                               (current-column)))
           (error nil)))
-       ;; inside a string and it starts before this line
+       ;; Inside a string and it starts before this line.
        ((and (nth 3 parse)
              (< (nth 8 parse) (save-excursion (beginning-of-line) (point))))
         (indent-line-to 0))
-       ;; inside a defun, but not a nested list (depth is 1)
+
+       ;; Inside a defun, but not a nested list (depth is 1).  This is
+       ;; a promise, usually.
+
+       ;; Indent to cfengine-indent times the nested depth
+       ;; plus 2.  That way, promises indent deeper than class
+       ;; selectors, which in turn are one deeper than categories.
        ((= 1 (nth 0 parse))
         (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))
        ;; Inside brackets/parens: indent to start column of non-comment
@@ -421,8 +451,8 @@
   (modify-syntax-entry ?\\ "." table))
 
 ;;;###autoload
-(define-derived-mode cfengine3-mode prog-mode "CFEngine3"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine3-mode prog-mode "CFE3"
+  "Major mode for editing CFEngine3 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
@@ -441,39 +471,39 @@
        #'cfengine3-end-of-defun))
 
 ;;;###autoload
-(define-derived-mode cfengine-mode prog-mode "Cfengine"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine2-mode prog-mode "CFE2"
+  "Major mode for editing CFEngine2 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
 to the action header."
   (cfengine-common-settings)
-  (cfengine-common-syntax cfengine-mode-syntax-table)
+  (cfengine-common-syntax cfengine2-mode-syntax-table)
 
   ;; Shell commands can be quoted by single, double or back quotes.
   ;; It's debatable whether we should define string syntax, but it
   ;; should avoid potential confusion in some cases.
-  (modify-syntax-entry ?\' "\"" cfengine-mode-syntax-table)
-  (modify-syntax-entry ?\` "\"" cfengine-mode-syntax-table)
+  (modify-syntax-entry ?\' "\"" cfengine2-mode-syntax-table)
+  (modify-syntax-entry ?\` "\"" cfengine2-mode-syntax-table)
 
-  (set (make-local-variable 'indent-line-function) #'cfengine-indent-line)
+  (set (make-local-variable 'indent-line-function) #'cfengine2-indent-line)
   (set (make-local-variable 'outline-regexp) "[ \t]*\\(\\sw\\|\\s_\\)+:+")
-  (set (make-local-variable 'outline-level) #'cfengine-outline-level)
+  (set (make-local-variable 'outline-level) #'cfengine2-outline-level)
   (set (make-local-variable 'fill-paragraph-function)
        #'cfengine-fill-paragraph)
-  (define-abbrev-table 'cfengine-mode-abbrev-table cfengine-mode-abbrevs)
+  (define-abbrev-table 'cfengine2-mode-abbrev-table cfengine-mode-abbrevs)
   (setq font-lock-defaults
-	'(cfengine-font-lock-keywords nil nil nil beginning-of-line))
+        '(cfengine2-font-lock-keywords nil nil nil beginning-of-line))
   ;; Fixme: set the args of functions in evaluated classes to string
   ;; syntax, and then obey syntax properties.
-  (setq imenu-generic-expression cfengine-imenu-expression)
+  (setq imenu-generic-expression cfengine2-imenu-expression)
   (set (make-local-variable 'beginning-of-defun-function)
-       #'cfengine-beginning-of-defun)
-  (set (make-local-variable 'end-of-defun-function) #'cfengine-end-of-defun))
+       #'cfengine2-beginning-of-defun)
+  (set (make-local-variable 'end-of-defun-function) #'cfengine2-end-of-defun))
 
 ;;;###autoload
 (defun cfengine-auto-mode ()
-  "Choose between `cfengine-mode' and `cfengine3-mode' depending
+  "Choose between `cfengine2-mode' and `cfengine3-mode' depending
 on the buffer contents"
   (let ((v3 nil))
     (save-restriction
@@ -481,7 +511,9 @@
       (while (not (or (eobp) v3))
         (setq v3 (looking-at (concat cfengine3-defuns-regex "\\>")))
         (forward-line)))
-    (if v3 (cfengine3-mode) (cfengine-mode))))
+    (if v3 (cfengine3-mode) (cfengine2-mode))))
+
+(defalias 'cfengine-mode 'cfengine-auto-mode)
 
 (provide 'cfengine3)
 (provide 'cfengine)

Re: changes to cfengine-mode

[Stefan Monnier]

> OK.  See attached, and I hope final, patch.

A few more nit picks, feel free to install the patch after addressing them.

> +(defcustom cfengine-mode-debug nil
> +  "Whether `cfengine-mode' should print debugging info
> + (only used by the cfengine3 support currently)"
> +  :group 'cfengine
> +  :type 'boolean)

We usually make those debug variables with `defvar' rather than `defcustom'.

>  (defcustom cfengine-mode-abbrevs nil
> -  "Abbrevs for Cfengine mode."
> +  "Abbrevs for CFEngine2 mode.
> +Obsolete, see `edit-abbrevs' instead."
[...]
> +(make-obsolete-variable 'cfengine-mode-abbrevs nil "24.1")

Don't mention "Obsolete" in the docstring, since the
make-obsolete-variable already gives this info.  And please put the
"use edit-abbrevs" as second argument to make-obsolete-variable instead.

>  (defvar cfengine3-font-lock-keywords
>    `(
> +    ;; defuns

Please capitalize your comments.

> +    (,(concat "\\<" cfengine3-defuns-regex "\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)\\((\\([^)]*\\))\\)")
> +    (1 font-lock-builtin-face)
> +    (2 font-lock-constant-face)
> +    (3 font-lock-function-name-face)
> +    (5 font-lock-variable-name-face))
> +
> +    (,(concat "\\<" cfengine3-defuns-regex "\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)")
> +    (1 font-lock-builtin-face)
> +    (2 font-lock-constant-face)
> +    (3 font-lock-function-name-face))

Why not merge the two entries into something like:

       (,(concat "\\<" cfengine3-defuns-regex "\\>"
                 "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
                 "[ \t]+\\<\\([[:alnum:]_]+\\)\\(?:(\\([^)]*\\))\\)?")
       (1 font-lock-builtin-face)
       (2 font-lock-constant-face)
       (3 font-lock-function-name-face)
       (4 font-lock-variable-name-face nil t))

And please make sure you explain in the commit log (or here in comments)
why this code was moved to the beginning of
cfengine3-font-lock-keywords.


        Stefan

Re: changes to cfengine-mode

[Ted Zlatanov]

On Fri, 16 Dec 2011 19:45:53 -0500 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> OK.  See attached, and I hope final, patch.
SM> A few more nit picks, feel free to install the patch after addressing them.

Done.  I left the Version header in; feel free to remove it if plans change.

SM> Why not merge the two entries into something like:

SM>        (,(concat "\\<" cfengine3-defuns-regex "\\>"
SM>                  "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
SM>                  "[ \t]+\\<\\([[:alnum:]_]+\\)\\(?:(\\([^)]*\\))\\)?")
SM>        (1 font-lock-builtin-face)
SM>        (2 font-lock-constant-face)
SM>        (3 font-lock-function-name-face)
SM>        (4 font-lock-variable-name-face nil t))

I thought my version was easier to read, but I used yours with an extra
comment to explain the last part.

SM> And please make sure you explain in the commit log (or here in
SM> comments) why this code was moved to the beginning of
SM> cfengine3-font-lock-keywords.

OK, done in the comments.

Ted