Re: Reify the cc-mode-common into an actual parent mode

[Alan Mackenzie]

Hello, Stefan.

On Mon, May 23, 2016 at 08:41:56PM -0400, Stefan Monnier wrote:
> IIRC last conversation, while you disagreed with introducing
> a c-derivative-mode (for objc/c++/c), you said you were OK with the idea
> of introducing a cc-mode-common.

Did I?

> Here's a corresponding patch.
> Any objection?

Just the same ones as we've already discussed.  The patch will fragment
the mode initialisations, making them more difficult to follow and
understand.  There's nothing coherent about what would be left in
`c-mode', for example: just a few random forms which happen not to be
shared by the other modes.

There's nothing coherent about `c-mode-common'; it isn't sensible to set
a buffer to this mode, and it would be erroneous to attempt to derive a
mode (other than the seven within CC Mode) directly from it, since the
language variables for the new mode wouldn't get initialised.  It's doc
string doesn't, as yet, make this latter point clear.  The canonical way
to create a mode derived from CC Mode is to derive from, say, `c-mode',
call `c-add-language', then specify the values of the language variables
which differ from those of `c-mode'.

`c-update-modeline' should be called as the last thing in mode
initialisation.  The way the patch is structured, it gets called before
`c-make-noise-macro-regexps' and `c-make-macro-with-semi-re' in those
modes that have them.  It so happens that, at the moment, those two
functions don't affect `c-update-modeline', so things work, but this
executing in the wrong order is storing up trouble for the future, should
some form in `c-mode''s :after-hook position need executing before
`c-update-modeline'.

`c-make-inherited-keymap' is not obsolete, being required for XEmacs
compatibility.  (XEmacs is still alive, if perhaps on life support.
SXEmacs is very much alive.)  The alternative suggested in the patch's
`make-obsolete' form most assuredly won't work in XEmacs, yet hackers
aren't warned about this there.

Likewise `c-run-mode-hooks' is not obsolete.  The suggestion instead to
derive directly from `c-common-mode' (note this should be
`c-mode-common') is simply wrong (see above).

But the main thing I don't like about the patch is that it's a lot of
work (for both of us), yet doesn't solve a single bug, and doesn't
implement any new feature.  The new `c-mode-common', which isn't a
coherent entity, is going to cause problems (and hence take up more time)
when somebody tries to derive directly from it.  Possibly there will be
other problems with the patch.

I'm really not keen on it.

>         Stefan


> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
> index de903b8..bbb96f2 100644
> --- a/lisp/progmodes/cc-mode.el
> +++ b/lisp/progmodes/cc-mode.el
> @@ -238,6 +238,8 @@ control).  See \"cc-mode.el\" for more info."
>       ;; incompatible
>       (t (error "CC Mode is incompatible with this version of Emacs")))
>      map))
> +(make-obsolete 'c-make-inherited-keymap
> +            "Use make-sparse-keymap + (set-keymap-parent c-mode-base-map); 
> or derive from c-mode-common" "26.1")
 
This function is not obsolete.

>  (defun c-define-abbrev-table (name defs &optional doc)
>    ;; Compatibility wrapper for `define-abbrev' which passes a non-nil
> @@ -861,6 +863,7 @@ Note that the style variables are always made local to 
> the buffer."
>    (if (cc-bytecomp-fboundp 'run-mode-hooks)
>        `(run-mode-hooks ,@hooks)
>      `(progn ,@(mapcar (lambda (hook) `(run-hooks ,hook)) hooks))))
> +(make-obsolete 'c-run-mode-hooks "derive your mode from c-common-mode" 
> "26.1")
 
This function is also not obsolete: deriving from `c-mode-common' (note
typo) is wrong (see above).

 
>  ;;; Change hooks, linking with Font Lock and electric-indent-mode.
> @@ -1431,6 +1434,17 @@ This function is called from `c-common-init', once per 
> mode initialization."
>      (c-update-modeline)))
 
 
> +(defvar c-mode-common-map c-mode-base-map)
> +
> +(define-derived-mode c-mode-common prog-mode "CC-common"
> +  "Pseudo major mode, parent of all modes using the CC engine."

NO!!!  It is the parent of C Mode, C++ Mode, ....., AWK Mode, but is most
definitely not the parent of modes derived from CC Mode (see above).

> +  ;; We want to update the mode-line but *after* the major mode's hooks
> +  ;; have been run.
> +  :after-hook (c-update-modeline)
> +  (c-initialize-cc-mode t)
> +  (setq abbrev-mode t)  ;; Used for c-electric-continued-statement!
> +  )
> +
>  ;; Support for C
 
>  (defvar c-mode-syntax-table
> @@ -1443,7 +1457,7 @@ This function is called from `c-common-init', once per 
> mode initialization."
>    "Abbreviation table used in c-mode buffers.")
 
>  (defvar c-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Add bindings which are only useful for C.
>      (define-key map "\C-c\C-e"  'c-macro-expand)
>      map)
> @@ -1484,7 +1498,7 @@ This function is called from `c-common-init', once per 
> mode initialization."
>  (unless (fboundp 'prog-mode) (defalias 'prog-mode 'fundamental-mode))
 
>  ;;;###autoload
> -(define-derived-mode c-mode prog-mode "C"
> +(define-derived-mode c-mode c-mode-common "C"
>    "Major mode for editing C code.
 
>  To submit a problem report, enter `\\[c-submit-bug-report]' from a
> @@ -1500,15 +1514,11 @@ initialization, then `c-mode-hook'.
>  Key bindings:
>  \\{c-mode-map}"
>    :after-hook (progn (c-make-noise-macro-regexps)
> -                  (c-make-macro-with-semi-re)
> -                  (c-update-modeline))
> -  (c-initialize-cc-mode t)
> -  (setq abbrev-mode t)
> +                  (c-make-macro-with-semi-re))
>    (c-init-language-vars-for 'c-mode)
>    (c-common-init 'c-mode)
>    (easy-menu-add c-c-menu)
> -  (cc-imenu-init cc-imenu-c-generic-expression)
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  (cc-imenu-init cc-imenu-c-generic-expression))
 
 
>  ;; Support for C++
> @@ -1524,7 +1534,7 @@ Key bindings:
>    "Abbreviation table used in c++-mode buffers.")
 
>  (defvar c++-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Add bindings which are only useful for C++.
>      (define-key map "\C-c\C-e" 'c-macro-expand)
>      (define-key map "\C-c:"    'c-scope-operator)
> @@ -1537,7 +1547,7 @@ Key bindings:
>                 (cons "C++" (c-lang-const c-mode-menu c++)))
 
>  ;;;###autoload
> -(define-derived-mode c++-mode prog-mode "C++"
> +(define-derived-mode c++-mode c-mode-common "C++"
>    "Major mode for editing C++ code.
>  To submit a problem report, enter `\\[c-submit-bug-report]' from a
>  c++-mode buffer.  This automatically sets up a mail buffer with
> @@ -1553,15 +1563,11 @@ initialization, then `c++-mode-hook'.
>  Key bindings:
>  \\{c++-mode-map}"
>    :after-hook (progn (c-make-noise-macro-regexps)
> -                  (c-make-macro-with-semi-re)
> -                  (c-update-modeline))
> -  (c-initialize-cc-mode t)
> -  (setq abbrev-mode t)
> +                  (c-make-macro-with-semi-re))
>    (c-init-language-vars-for 'c++-mode)
>    (c-common-init 'c++-mode)
>    (easy-menu-add c-c++-menu)
> -  (cc-imenu-init cc-imenu-c++-generic-expression)
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  (cc-imenu-init cc-imenu-c++-generic-expression))
 
 
>  ;; Support for Objective-C
> @@ -1576,7 +1582,7 @@ Key bindings:
>    "Abbreviation table used in objc-mode buffers.")
 
>  (defvar objc-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Add bindings which are only useful for Objective-C.
>      (define-key map "\C-c\C-e" 'c-macro-expand)
>      map)
> @@ -1588,7 +1594,7 @@ Key bindings:
>  ;;;###autoload (add-to-list 'auto-mode-alist '("\\.m\\'" . objc-mode))
 
>  ;;;###autoload
> -(define-derived-mode objc-mode prog-mode "ObjC"
> +(define-derived-mode objc-mode c-mode-common "ObjC"
>    "Major mode for editing Objective C code.
>  To submit a problem report, enter `\\[c-submit-bug-report]' from an
>  objc-mode buffer.  This automatically sets up a mail buffer with
> @@ -1604,15 +1610,11 @@ initialization, then `objc-mode-hook'.
>  Key bindings:
>  \\{objc-mode-map}"
>    :after-hook (progn (c-make-noise-macro-regexps)
> -                  (c-make-macro-with-semi-re)
> -                  (c-update-modeline))
> -  (c-initialize-cc-mode t)
> -  (setq abbrev-mode t)
> +                  (c-make-macro-with-semi-re))
>    (c-init-language-vars-for 'objc-mode)
>    (c-common-init 'objc-mode)
>    (easy-menu-add c-objc-menu)
> -  (cc-imenu-init nil 'cc-imenu-objc-function)
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  (cc-imenu-init nil 'cc-imenu-objc-function))
 
 
>  ;; Support for Java
> @@ -1629,7 +1631,7 @@ Key bindings:
>    "Abbreviation table used in java-mode buffers.")
 
>  (defvar java-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Add bindings which are only useful for Java.
>      map)
>    "Keymap used in java-mode buffers.")
> @@ -1647,7 +1649,7 @@ Key bindings:
>  ;;;###autoload (add-to-list 'auto-mode-alist '("\\.java\\'" . java-mode))
 
>  ;;;###autoload
> -(define-derived-mode java-mode prog-mode "Java"
> +(define-derived-mode java-mode c-mode-common "Java"
>    "Major mode for editing Java code.
>  To submit a problem report, enter `\\[c-submit-bug-report]' from a
>  java-mode buffer.  This automatically sets up a mail buffer with
> @@ -1662,14 +1664,10 @@ initialization, then `java-mode-hook'.
 
>  Key bindings:
>  \\{java-mode-map}"
> -  :after-hook (c-update-modeline)
> -  (c-initialize-cc-mode t)
> -  (setq abbrev-mode t)
>    (c-init-language-vars-for 'java-mode)
>    (c-common-init 'java-mode)
>    (easy-menu-add c-java-menu)
> -  (cc-imenu-init cc-imenu-java-generic-expression)
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  (cc-imenu-init cc-imenu-java-generic-expression))
 
 
>  ;; Support for CORBA's IDL language
> @@ -1682,7 +1680,7 @@ Key bindings:
>    "Abbreviation table used in idl-mode buffers.")
 
>  (defvar idl-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Add bindings which are only useful for IDL.
>      map)
>    "Keymap used in idl-mode buffers.")
> @@ -1693,7 +1691,7 @@ Key bindings:
>  ;;;###autoload (add-to-list 'auto-mode-alist '("\\.idl\\'" . idl-mode))
 
>  ;;;###autoload
> -(define-derived-mode idl-mode prog-mode "IDL"
> +(define-derived-mode idl-mode c-mode-common "IDL"
>    "Major mode for editing CORBA's IDL, PSDL and CIDL code.
>  To submit a problem report, enter `\\[c-submit-bug-report]' from an
>  idl-mode buffer.  This automatically sets up a mail buffer with
> @@ -1708,13 +1706,13 @@ initialization, then `idl-mode-hook'.
 
>  Key bindings:
>  \\{idl-mode-map}"
> -  :after-hook (c-update-modeline)
> -  (c-initialize-cc-mode t)
> +  ;; No c-electric-continued-statement here, so we don't need abbrev-mode.
> +  (kill-local-variable 'abbrev-mode)
>    (c-init-language-vars-for 'idl-mode)
>    (c-common-init 'idl-mode)
>    (easy-menu-add c-idl-menu)
>    ;;(cc-imenu-init cc-imenu-idl-generic-expression) ;TODO
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  )
 
 
>  ;; Support for Pike
> @@ -1729,7 +1727,7 @@ Key bindings:
>    "Abbreviation table used in pike-mode buffers.")
 
>  (defvar pike-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Additional bindings.
>      (define-key map "\C-c\C-e" 'c-macro-expand)
>      map)
> @@ -1742,7 +1740,7 @@ Key bindings:
>  ;;;###autoload (add-to-list 'interpreter-mode-alist '("pike" . pike-mode))
 
>  ;;;###autoload
> -(define-derived-mode pike-mode prog-mode "Pike"
> +(define-derived-mode pike-mode c-mode-common "Pike"
>    "Major mode for editing Pike code.
>  To submit a problem report, enter `\\[c-submit-bug-report]' from a
>  pike-mode buffer.  This automatically sets up a mail buffer with
> @@ -1757,14 +1755,11 @@ initialization, then `pike-mode-hook'.
 
>  Key bindings:
>  \\{pike-mode-map}"
> -  :after-hook (c-update-modeline)
> -  (c-initialize-cc-mode t)
> -  (setq      abbrev-mode t)
>    (c-init-language-vars-for 'pike-mode)
>    (c-common-init 'pike-mode)
>    (easy-menu-add c-pike-menu)
>    ;;(cc-imenu-init cc-imenu-pike-generic-expression) ;TODO
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  )
 
 
>  ;; Support for AWK
> @@ -1781,7 +1776,7 @@ Key bindings:
>    "Abbreviation table used in awk-mode buffers.")
 
>  (defvar awk-mode-map
> -  (let ((map (c-make-inherited-keymap)))
> +  (let ((map (make-sparse-keymap)))
>      ;; Add bindings which are only useful for awk.
>      (define-key map "#" 'self-insert-command);Overrides electric parent 
> binding.
>      (define-key map "/" 'self-insert-command);Overrides electric parent 
> binding.
> @@ -1804,7 +1799,7 @@ Key bindings:
>  (declare-function c-awk-unstick-NL-prop "cc-awk" ())
 
>  ;;;###autoload
> -(define-derived-mode awk-mode prog-mode "AWK"
> +(define-derived-mode awk-mode c-mode-common "AWK"
>    "Major mode for editing AWK code.
>  To submit a problem report, enter `\\[c-submit-bug-report]' from an
>  awk-mode buffer.  This automatically sets up a mail buffer with version
> @@ -1818,18 +1813,14 @@ initialization, then `awk-mode-hook'.
 
>  Key bindings:
>  \\{awk-mode-map}"
> -  :after-hook (c-update-modeline)
>    ;; We need the next line to stop the macro defining
>    ;; `awk-mode-syntax-table'.  This would mask the real table which is
>    ;; declared in cc-awk.el and hasn't yet been loaded.
>    :syntax-table nil
>    (require 'cc-awk)                  ; Added 2003/6/10.
> -  (c-initialize-cc-mode t)
> -  (setq      abbrev-mode t)
>    (c-init-language-vars-for 'awk-mode)
>    (c-common-init 'awk-mode)
> -  (c-awk-unstick-NL-prop)
> -  (c-run-mode-hooks 'c-mode-common-hook))
> +  (c-awk-unstick-NL-prop))
 
 
>  ;; bug reporting

-- 
Alan Mackenzie (Nuremberg, Germany).